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

Problem accessing to the request body after configure Sentry in FastAPI #1573

Closed
LuisMlopez opened this issue Aug 19, 2022 · 13 comments · Fixed by #1649
Closed

Problem accessing to the request body after configure Sentry in FastAPI #1573

LuisMlopez opened this issue Aug 19, 2022 · 13 comments · Fixed by #1649

Comments

@LuisMlopez
Copy link

LuisMlopez commented Aug 19, 2022

Steps to Reproduce

  1. Create a Middleware to log the request body like https://stackoverflow.com/questions/70134618/why-does-fastapi-hang-when-adding-a-middleware-to-print-the-http-request-body
  2. The previous step works before configuring the Sentry SDK
  3. Configure the Sentry as defined in https://docs.sentry.io/platforms/python/guides/fastapi/
  4. When try to access to the test URL, the application hangs because the request body was already accessed by the Sentry integration

Expected Result

Simplified middleware example

@app.middleware("http")
async def logging_middleware(request: Request, call_next):
    start_time = time.time()
    body = await request.body()
    print(body)
    response = await call_next(request)`
    return response

Actual Result

When the Sentry implementation is active, the body = await request.body() line hangs and never response.

Additional Info

This answer has more information related to the problem https://stackoverflow.com/a/71811390/11030205

@getsentry-release
Copy link

Routing to @getsentry/team-web-sdk-backend for triage. ⏲️

@vladanpaunovic vladanpaunovic transferred this issue from getsentry/sentry-docs Aug 19, 2022
@vladanpaunovic
Copy link
Contributor

Thanks for submitting this issue @LuisMlopez.

Our FastAPI integration is still in alpha so this feedback is very valuable! We will look into this in about two weeks time.

@antonpirker
Copy link
Member

Hey @LuisMlopez !
I have added your example middleware to a demo project of mine, but the body = await request.body() is not blocking, it is the line response = await call_next(request) that is blocking for me. (An when debugging, this is the line in Starlette that is acutally blocking: https://github.com/encode/starlette/blob/master/starlette/middleware/base.py#L43)

This is because you can only read the request body once, because in Starlette the request body is a stream. This is why Starlette is caching the body in a local variable _body when reading the body from the stream:
https://github.com/encode/starlette/blob/master/starlette/requests.py#L230-L238

But the BaseHTTPMiddleware that is called here by your demo middleware (through call_next()) does not use the Starlette Request object so there is no caching. Hence it hangs.

I do not yet have an idea how to fix this. Let me thing about it (and ask in the Starlette repo)

@antonpirker
Copy link
Member

Hello again @LuisMlopez

I have now found out, that the application hanging when one tries to read the request body twice (for example once with sentry and then again witha in a middleware, as in our example above) is a known limitation. See: encode/starlette#1729

The middleware in our example is base upon BaseHTTPMiddleware and this tries to read the body data with request.receive() which hangs because the body was already read from Sentry with starlettes Request class.

Would Starlettes BaseHTTPMiddleware use also Starlettes Request class it would work, because the class does cache the body data in memory.

But I guess right now, there is nothing we can do about. There are discussions going on in the Starlette project on how to fix this limitation with BaseHTTPMiddleware but as this is going on since begin of June I guess this will not be resolved in the next couple of weeks.

@antonpirker
Copy link
Member

I have now asked for help in the Starlette community, maybe somebody has an idea on how to solve this:
encode/starlette#1729 (comment)

@Kludex
Copy link

Kludex commented Aug 29, 2022

This is an issue on Sentry's side. We'll explain, and propose some solution(s). cc @adriangb

EDIT: @adriangb explained the possible solutions: encode/starlette#1729 (reply in thread)

@antonpirker
Copy link
Member

Thanks for the feedback @Kludex. I will have a look!

@LuisMlopez
Copy link
Author

LuisMlopez commented Sep 1, 2022

Hi @antonpirker

Thanks a lot for all the information.

I have found a temporal solution to use sentry and the logging middleware together. I know that is not the best approach but it works and can be used while the problem reported here is not solved:

import sentry_sdk

@app.middleware("http")
async def logging_middleware(request: Request, call_next):
    start_time = time.time()
    body = await request.body()
    print(body)
    try:
        response = await call_next(request)`
    except Exception as exception:
        with sentry_sdk.push_scope() as scope:
            scope.set_context("request", request)
            sentry_sdk.capture_exception(exception)

        return JSONResponse(
            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
            content={
                "detail": "An internal error has occurred while processing the request."
            },
        )

    return response

All the exceptions will be logged in Sentry "manually", so the Sentry setup described here https://docs.sentry.io/platforms/python/guides/fastapi/ is not required.

I hope this solution can help others with the same problem.

@adriangb
Copy link

adriangb commented Sep 1, 2022

@antonpirker I just gave a look at the Sentry middleware again to try to decipher exactly what's going on. I originally though that Sentry was reading the request/response in a middleware, but I'm realizing now it's doing that at the endpoint level. I think this means the order of execution is user middleware -> sentry extractor -> endpoint, right?

@antonpirker antonpirker added this to the FastAPI/Starlette Support milestone Sep 15, 2022
@antonpirker
Copy link
Member

I think this issue is also solved by fixing this one: #1631

@antonpirker
Copy link
Member

(We read the request body for determining the size of it. (Which is really a stupid thing to do). Now we read the content length from the request headers and therefore do not touch the request body twice)

@Kludex
Copy link

Kludex commented Oct 3, 2022

And if the request doesn't have a content-length?

@antonpirker
Copy link
Member

In that case we just ignore the request body, because we can not be sure to not break the users code when accessing it.

antonpirker added a commit that referenced this issue Oct 3, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
, #1631, #1595, #1573)

* Do not read request body to determine content length.
* Made AnnotatedValue understandable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants