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

Support for large requests (more_body=True) #11

Open
perlman opened this issue Aug 24, 2020 · 5 comments
Open

Support for large requests (more_body=True) #11

perlman opened this issue Aug 24, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@perlman
Copy link

perlman commented Aug 24, 2020

This library seemed to hit the spot for drop-in support for msgpack with FastAPI. I am using the following to enable the msgpack interface:
app.add_middleware(MessagePackMiddleware)

Unfortunately, large client requests are failing. Running uvicorn with --log-level trace I see that the request is being chunked:

TRACE:    127.0.0.1:42088 - Connection made
TRACE:    10.60.1.118:0 - ASGI [4] Started scope={'type': 'http', 'asgi': {'version': '3.0', 'spec_version': '2.1'}, 'http_version': '1.0', 'server': ('127.0.0.1', 8901), 'client': ('10.60.1.118', 0), 'scheme': 'https', 'method': 'POST', 'root_path': '/app/<redacted>', 'path': '<redacted> 'raw_path': b'/<redacted>', 'query_string': b'', 'headers': '<...>'}
TRACE:    10.60.1.118:0 - ASGI [4] Receive {'type': 'http.request', 'body': '<65150 bytes>', 'more_body': True}
TRACE:    10.60.1.118:0 - ASGI [4] Receive {'type': 'http.request', 'body': '<65482 bytes>', 'more_body': True}
TRACE:    10.60.1.118:0 - ASGI [4] Send {'type': 'http.response.start', 'status': 400, 'headers': '<...>'}
INFO:     10.60.1.118:0 - "POST<redacted> HTTP/1.0" 400 Bad Request
TRACE:    10.60.1.118:0 - ASGI [4] Send {'type': 'http.response.body', 'body': '<45 bytes>'}
TRACE:    10.60.1.118:0 - ASGI [4] Completed
TRACE:    127.0.0.1:42088 - Connection lost

Requests under 64k work fine. JSON requests of any size are also fine.
The request is being sent from the client as a regular POST.

I assume this is related the comments about more_body not being implemented in the source code.

I am still getting up to speed with ASGI. Is this something that should be fixed with this middleware, or should I look at figuring out how to increase the buffer size elsewhere?

@florimondmanca
Copy link
Owner

Hi!

Interesting... Could you share a minimal ASGI app (FastAPI or otherwise) along with a sample request that allow to reproduce this issue? We'd need to make sure this doesn't come from interaction with eg some FastAPI feature, or external limitations such as body limits on a reverse proxy. Eg I'm not sure where the 400 comes from...

@florimondmanca
Copy link
Owner

florimondmanca commented Aug 25, 2020

About the 400: we raise a NotImplementedError in our code so I would be expecting the traceback from that plus a 500 error response, not a 400 bad request response. See: https://github.com/florimondmanca/msgpack-asgi/blob/master/src/msgpack_asgi/_middleware.py#L61

In any case yup, "streaming the request body is not supported yet", so we can discuss ways of making it supported. :) I'm not sure what the expected interaction w/ msgpack and JSON parsers would be - certainly we'll need to buffer up everything in the middleware, right?

@florimondmanca florimondmanca added the enhancement New feature or request label Aug 25, 2020
@perlman
Copy link
Author

perlman commented Aug 25, 2020

Thanks for the quick response! I'm quite new to FastAPI+ASGI, and finding any intuition I had with Flask only goes so far. ;-)

Here's a minimal app to show this:

server.py:

from fastapi import FastAPI
from msgpack_asgi import MessagePackMiddleware
from pydantic import BaseModel
from typing import List

app = FastAPI()

app.add_middleware(MessagePackMiddleware)

class StrList(BaseModel):
    hello: List[str]

@app.post("/")
def hello(data: StrList):
    return data

client.py:

import msgpack
import requests

# This works (msgpack, small)
url = "http://127.0.0.1:8000/"
data = { 'hello' : ['world'] }
headers = {'content-type': 'application/x-msgpack',
            'accept' : 'application/x-msgpack'}
resp = requests.post(url, data=msgpack.packb(data), headers=headers)
print(resp.status_code)

# This works (json, large)
data = { 'hello' : ['world'] * 50000 }
resp = requests.post(url, json=data)
print(resp.status_code)

# This doesn't (msgpack, large)
data = { 'hello' : ['world'] * 50000 }
headers = {'content-type': 'application/x-msgpack',
            'accept' : 'application/x-msgpack'}
resp = requests.post(url, data=msgpack.packb(data), headers=headers)
print(resp.status_code)
print(msgpack.unpackb(resp.content))

Running the app with uvicorn --reload msgpackbug:app

Client Output:

200
200
400
{'detail': 'There was an error parsing the body'}

@florimondmanca
Copy link
Owner

florimondmanca commented Aug 25, 2020

Thanks!

Although it's clear there's action to take on our side to add support for streaming request bodies, I'm still curious where this 400 comes from. I guess the next step would be to determine which component in the stack (among FastAPI, Uvicorn, msgpack-asgi) does the checking that leads to this "failed to parse the body" error...?

@amircodota
Copy link

If anyone's interested, I implemented this middleware, which I put infront of the msgpack middleware.

It does the buffering in memory.
Seems to work in my case

class FullBodyMiddleware(BaseHTTPMiddleware):
    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        if scope["type"] != "http":
            await super().__call__(scope, receive, send)
            return

        async def full_receive():
            message = await receive()

            full_body = message["body"]
            more = message.get("more_body", False)

            while more:
                next_message = await receive()
                full_body += next_message["body"]
                more = next_message.get("more_body", False)

            message["body"] = full_body
            message["more_body"] = False
            return message

        await super().__call__(scope, full_receive, send)

    async def dispatch(
        self, request: Request, call_next: RequestResponseEndpoint
    ) -> Response:
        return await call_next(request)

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

No branches or pull requests

3 participants