Fixes Request.json() Hangs Forever

[nikordaris]

This fixes #847 by using the Request scope to store consumed stream data so that new instances of Request can access the Request stream content. Previously await Request(...).json() followed by another await Request(...).json() would hang forever because the receive stream was exhausted and await receive() doesn't timeout after message {"more_body": False} is received. This PR is focused only on enabling Request.body(), Request.form() and Request.json() to return the cached data as originally intended and does not address the issue of await receive() hanging forever inside of Request.stream()

[lwinkler-cloudflight]

is there a reason nobody had a look at this yet? seems fairly straight forward and fixes an issue that we keep stumbling over.

[akarpodinis]

Any love for this issue? It's been quiet in here for months.

[graingert]

is this fixed by the anyio port?

[pawelswiecki]

Any updates?

[gnat]

Would be really nice to see this go in so multiple calls to await request.form() etc. are managed automatically--- I'm currently storing the result in request.state in the interim so I can make use of it in middlewares, as well as endpoints.

[gnat]

Possible to give this a closer look? @tomchristie This would close the following tickets:

• Consume request body in middleware is problematic #495
• Cannot access POST data from AuthenticationBackend #771
• Middleware Request parse Hangs forever #847
• Question about Request.Body() #493

[moonso]

Are there any progress or discussions going on around this?

[Kludex]

@nikordaris Are you still interested in helping with this PR?

[nikordaris]

I left a comment on Tom's comment. From my perspective I'm not seeing anything else for me to change with this PR. Let me know if there is anything simple I can tweak on this PR to help it get merged. As I'm no longer on this tech stack I won't likely have time to do any major modifications to this PR

[nikordaris]

Ok. Where did you want the cache stored in the scope? In extensions? Directly on the scope? If extensions what namespace? starlette? request_cache? @tomchristie

I'll do one more pass at this and then I'm done. I'm not on this tech stack anymore and cant invest any more time on this. Someone else will have to fork my PR and resubmit it with any follow-up changes.

My approach will be to cache the raw stream only for body and form. If stream is used directly it will use the cache if it exists. If the stream is consumed and there is no cache then it will raise a runtime error.

[nikordaris]

Ok, @tomchristie this is ready for review. The Request.stream no longer caches. If Request.body or Request.form is called it will cache the raw bytes stream into scope.extensions.starlette.stream. If Request.stream is called followed by Request.body, a runtime error will be thrown indicating the stream has been consumed. If Request.body or Request.form is called followed by Request.stream, no error is raised and the cached stream will be returned. I think this lines up with your concerns around a large stream getting stored in memory while still preserving the convenience of leveraging the cache from body or form. Once you decide where in the scope you want to store the cached stream, i'll update that location and then this PR should be complete.

[tomchristie]

Just bumping by to mention that this is on my personal backlog at the moment, but I don't want to deal with it until I can give it some proper attention, since it's a bit of a fiddly area.

Please do feel free to nudge me next week if I don't already get onto looking at it before then.

[tomchristie]

Okay, so I'd been pushing back against this approach because it changes the nature of streaming the request.

If we cache all the data we get when we stream a request, then we're always going to store the entire body in memory. That's find in cases where you're accessing the body (because you needed it all in memory in that cases), or in cases where you're accessing the request as JSON (because, same). Tho it's not ideal in cases where you're accessing request.form(), because in that case we're careful that the request parsing deliberately doesn't store the entire body in memory.

There's an alternate approach that'd essentially be to raise an error there's an attempt to stream the request more than once but that does cache the result of .body and .form.

However at this point, I'm kinda thinking that we should possibly just suck it up, and accept the trade-off because it results in a much simpler set of "here's what you can and can't do when repeatedly accessing the request content".

Anyone?

[adriangb]

If we cache all the data we get when we stream a request, then we're always going to store the entire body in memory. That's fine in cases where you're accessing the body (because you needed it all in memory in that cases), or in cases where you're accessing the request as JSON (because, same). Tho it's not ideal in cases where you're accessing request.form(), because in that case we're careful that the request parsing deliberately doesn't store the entire body in memory.

I think I agree with what you say, but I'm not understanding the relationship to this PR.
Let me echo back to you my interpretation to see if I'm missing anything.

My understanding is that currently in Starlette if you do Request.body() (or anything that calls that, like Request.json()) the body gets cached. On the other hand, if you do Request.stream() (or anything that calls that, like Request.form()) then the body is not cached in memory. From my understanding of this PR that is preserved, the only difference is that when the body is cached it is cached into the ASGI scope instead of the Request instance. Right @nikordaris ?

More generally, I think it would make sense to have an API like Request.form(consume=False) and Request.json(consume=True) (with defaults to the current behavior) that allow users to control this. That would I think aleviate confusion because instead of having to dig into the implementation details of Request.json or Request.form (or look at some footnote in the docs) you can see just from the function signature what's going on. Not only that, but if you're looking for this info it's probably because you want the opposite behavior, so you also already know how to change it. But again, this would have to be another PR.

[tomchristie]

Okay - this looks good - I misunderstood slightly on first pass. (Thanks for clearing that up for me @adriangb)

I think we also need to be careful to preserve the RuntimeError("Stream consumed") case.

I've created #1519 as a similar approach that also handles the stream consumed state.

[nikordaris]

@tomchristie it looks like this PR should be closed out in favor of #1519 now, correct?

[gnat]

Thank you @nikordaris for his valiant effort on this!! Looking forward to seeing the resolution to this in #1519