Allow configuring max_incomplete_event_size for h11 implementation #1240
Conversation
|
@Kludex I've added a test to demonstrate how h11 fails and another to show how the parameter fixes it. I wanted to actually assert the right exception, but the mock doesn't let it bubble up. Can you take a look and let me know what you think of this approach? |
…n and the h11_max_incomplete_event_size parameter
| max_incomplete_event_size=config.h11_max_incomplete_event_size, | ||
| ) | ||
| else: | ||
| self.conn = h11.Connection(h11.SERVER) |
There was a problem hiding this comment.
Maybe we should set the default ourselves instead of using the one h11 defines? Even if it's with the same value?
Just a thought.
There was a problem hiding this comment.
I think it's better to respect the h11 default in case they change it for some reason.
There was a problem hiding this comment.
I think the reason you mentioned is exactly why we should set it here 😅
Maybe we should check what hypercorn is doing here 🤔
There was a problem hiding this comment.
Hypercorn sets it, but I think it's the wrong approach. If you're using h11, you should trust them to set their defaults correctly. If they change it, they probably have a good reason for doing so, like fixing a bug or implementing a new feature. If you're still using the old default, now you're affected by the bug they already fixed and you have to update your default as well.
Anyway, if you feel strongly about this, feel free to set the default directly. The code will certainly be cleaner.
There was a problem hiding this comment.
If they change it, they probably have a good reason for doing so, like fixing a bug or implementing a new feature.
I think we need to be in charge here. h11 can change their values as they want, they are on pre-stable release (I doubt they would, because this value is actually based on several other implementations).
Also, the code becomes cleaner anyway.
Would you mind changing it?
|
btw, thanks for the PR! 🎉 |
Ok, done. |
Can't you do something like: |
| @@ -50,6 +50,7 @@ By default Uvicorn uses simple changes detection strategy that compares python f | |||
| * `--ws-ping-interval <float>` - Set the WebSockets ping interval, in seconds. Please note that this can be used only with the default `websockets` protocol. | |||
| * `--ws-ping-timeout <float>` - Set the WebSockets ping timeout, in seconds. Please note that this can be used only with the default `websockets` protocol. | |||
| * `--lifespan <str>` - Set the Lifespan protocol implementation. **Options:** *'auto', 'on', 'off'.* **Default:** *'auto'*. | |||
| * `--h11-max-incomplete-event-size <int>` - For h11 protocol implementation, set the maximum number of bytes to buffer of an incomplete event. | |||
There was a problem hiding this comment.
| * `--h11-max-incomplete-event-size <int>` - For h11 protocol implementation, set the maximum number of bytes to buffer of an incomplete event. | |
| * `--h11-max-incomplete-event-size <int>` - Set the maximum number of bytes to buffer of an incomplete event. Only available for `h11` HTTP protocol implementation. |
|
Not really, no. Feel free to close it. |
|
Any plans to merge this one? |
|
Let's reevaluate over the issue. |
Solves #1234.