Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reraise exception from background task #2696

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Sep 21, 2024

Currently, the exceptions on the background tasks are swallowed:

from typing import Any

from starlette.applications import Starlette
from starlette.background import BackgroundTask
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.requests import Request
from starlette.responses import JSONResponse
from starlette.routing import Route


class CustomHeaderMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next: Any):
        response = await call_next(request)
        return response


async def bg_task():
    print("hi there")
    raise ValueError("TEST")


def homepage(request: Request):
    task = BackgroundTask(bg_task)
    message = {"status": "Ok"}
    return JSONResponse(message, background=task)


routes = [Route("/", homepage)]

middleware = [Middleware(CustomHeaderMiddleware)]

app = Starlette(routes=routes, middleware=middleware)

return {
"type": "http.request",
"body": self._body,
"more_body": False,
}
return {"type": "http.request", "body": self._body, "more_body": False}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this. It was just getting me crazy.

Comment on lines -73 to +69
return {
"type": "http.request",
"body": b"",
"more_body": False,
}
return {"type": "http.request", "body": b"", "more_body": False}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this. It was just getting me crazy.

Comment on lines -84 to +76
return {
"type": "http.request",
"body": chunk,
"more_body": not self._stream_consumed,
}
return {"type": "http.request", "body": chunk, "more_body": not self._stream_consumed}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this. It was just getting me crazy.

Comment on lines +139 to +140
# import traceback
# traceback.print_exc()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you uncomment those lines, you can see that we have the exceptions on the app_exc, but we don't reraise them at any point.

@@ -175,6 +165,8 @@ async def body_stream() -> typing.AsyncGenerator[bytes, None]:
if not message.get("more_body", False):
break

await anyio.sleep(0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is enough to make the switch so we have time to run the coro task after the break above.

This shouldn't be the way to solve this issue... Also, it breaks one of the tests.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this only work because in the test case in the PR description, the background task that fails isn't really async so a single task switch (via sleep(0)) is enough to trigger the exception, but a background task that does await would still not be triggered by this?

Assuming await self.app finishes all background tasks, the coro that awaits the app here could signal an event that can be awaited here instead.

@Kludex Kludex changed the title Raise exception from background task Reraise exception from background task Sep 21, 2024
@Kludex
Copy link
Member Author

Kludex commented Sep 29, 2024

@jhominal Any idea here?

@adriangb
Copy link
Member

adriangb commented Oct 2, 2024

@Kludex would #2176 resolve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

middleware causes exceptions to not be raised/handled silently (back again)
3 participants