You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@burr.apache.org by "sangkyoonnam (via GitHub)" <gi...@apache.org> on 2026/10/04 04:47:11 UTC
Re: [PR] fix: report execute hook failures [burr]
sangkyoonnam commented on code in PR #897:
URL: https://github.com/apache/burr/pull/897#discussion_r4176183225
##########
burr/core/application.py:
##########
@@ -746,23 +754,74 @@ def __call__(self, fn: CallableT) -> CallableT:
@functools.wraps(fn)
async def wrapper_async(app_self, *args, **kwargs):
# We only run at the top level, so we decorate it if we're there
- await self.acall_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = await self.acall_pre(app_self)
return await fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- await self.acall_post(app_self, exc)
+ if should_run_hooks:
+ await self.acall_post(app_self, exc)
@functools.wraps(fn)
def wrapper_sync(app_self, *args, **kwargs):
- self.call_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = self.call_pre(app_self)
return fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- self.call_post(app_self, exc)
+ if should_run_hooks:
+ self.call_post(app_self, exc)
- return wrapper_async if inspect.iscoroutinefunction(fn) else wrapper_sync
+ @functools.wraps(fn)
+ def wrapper_generator(app_self, *args, **kwargs):
+ def generator():
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
+ try:
+ should_run_hooks = self.call_pre(app_self)
Review Comment:
The sentinel set here stays in the caller's context while the generator is suspended, so other calls on the same app skip their execute hooks.
```python
gen = app.iterate(halt_after=["c"])
next(gen)
app.step()
```
Expected `pre`/`post` for `step` (main does that). Observed none. `aiterate` plus `astep()` and an open `iterate` plus `app.run()` behave the same way. One option: set `_run_call_var` around each `next(inner)` and reset it before yielding. `call_post` checks for the matching sentinel, so the final post call would need it set again for that call.
##########
burr/core/application.py:
##########
@@ -746,23 +754,74 @@ def __call__(self, fn: CallableT) -> CallableT:
@functools.wraps(fn)
async def wrapper_async(app_self, *args, **kwargs):
# We only run at the top level, so we decorate it if we're there
- await self.acall_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = await self.acall_pre(app_self)
return await fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- await self.acall_post(app_self, exc)
+ if should_run_hooks:
+ await self.acall_post(app_self, exc)
@functools.wraps(fn)
def wrapper_sync(app_self, *args, **kwargs):
- self.call_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = self.call_pre(app_self)
return fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- self.call_post(app_self, exc)
+ if should_run_hooks:
+ self.call_post(app_self, exc)
- return wrapper_async if inspect.iscoroutinefunction(fn) else wrapper_sync
+ @functools.wraps(fn)
+ def wrapper_generator(app_self, *args, **kwargs):
+ def generator():
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
+ try:
+ should_run_hooks = self.call_pre(app_self)
+ return (yield from fn(app_self, *args, **kwargs))
+ except BaseException as e:
+ exc = e
+ raise
+ finally:
+ if should_run_hooks:
+ self.call_post(app_self, exc)
+
+ return generator()
+
+ @functools.wraps(fn)
+ def wrapper_async_generator(app_self, *args, **kwargs):
+ async def generator():
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
+ try:
+ should_run_hooks = await self.acall_pre(app_self)
+ async for item in fn(app_self, *args, **kwargs):
Review Comment:
With `async for ... break` and no `aclose()`, asyncio finalizes the generator in a separate task, so the cleanup never reaches the caller's context.
```python
async for _ in app.aiterate(halt_after=["c"]):
break
gc.collect()
for _ in range(3):
await asyncio.sleep(0)
await app.astep()
```
The later `astep()` runs no execute hooks. With the OTel bridge, OTel logs `Failed to detach context` (`ValueError: ... created in a different Context`), no `astep` span is emitted, and the action span is parented to the already-ended `aiterate` span. On main both `astep` calls get their own root span. Scoping the sentinel per resume would likely fix the hook side. I haven't verified a fix for the OTel side.
##########
burr/core/application.py:
##########
@@ -746,23 +754,74 @@ def __call__(self, fn: CallableT) -> CallableT:
@functools.wraps(fn)
async def wrapper_async(app_self, *args, **kwargs):
# We only run at the top level, so we decorate it if we're there
- await self.acall_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = await self.acall_pre(app_self)
return await fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- await self.acall_post(app_self, exc)
+ if should_run_hooks:
+ await self.acall_post(app_self, exc)
@functools.wraps(fn)
def wrapper_sync(app_self, *args, **kwargs):
- self.call_pre(app_self)
- exc = None
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
try:
+ should_run_hooks = self.call_pre(app_self)
return fn(app_self, *args, **kwargs)
+ except BaseException as e:
+ exc = e
+ raise
finally:
- self.call_post(app_self, exc)
+ if should_run_hooks:
+ self.call_post(app_self, exc)
- return wrapper_async if inspect.iscoroutinefunction(fn) else wrapper_sync
+ @functools.wraps(fn)
+ def wrapper_generator(app_self, *args, **kwargs):
+ def generator():
+ should_run_hooks = False
+ exc: Optional[BaseException] = None
+ try:
+ should_run_hooks = self.call_pre(app_self)
+ return (yield from fn(app_self, *args, **kwargs))
+ except BaseException as e:
Review Comment:
`GeneratorExit` lands here, so a normal early exit is reported as a failure.
```python
for _ in app.iterate(halt_after=["c"]):
break
```
On main the root `iterate` span is `OK`. On this branch it's `ERROR`, and `post_run_execute_call` gets `exception=GeneratorExit`. `aiterate` followed by `aclose()` does the same. Suggest `except GeneratorExit: raise` ahead of this clause so `exc` stays `None`; same at line 809.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@burr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org