Repository navigation
HTTP/2 refactoring - #612
Conversation
|
I've given this a run around and it's looking really good to me so far. Hanging on www.yahoo.com, which I can't figure out - I don't recall if we'd resolved that fully or not. Haven't tested with 0.8 so see if it's a regression or not. |
I think you're looking for #236. You memory served you well, it's not a regression as the issue was only partially resolved. |
|
Btw. I'd recommend reviewing by looking at the new implementation itself rather than reviewing the diff. |
|
Note that this change will also let use drop TimeoutFlags and fork, given that we’ve got a nice simple “send the request, read the response” flow. As it turns out we just don’t need to perform concurrent read/writes. As a result it’ll also give use sync support for HTTP/2. |
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
This new implementation is so satisfying to read through. Great job! 👏
| local_flow = self.state.local_flow_control_window(stream_id) | ||
| connection_flow = self.state.max_outbound_frame_size | ||
| flow = min(local_flow, connection_flow) | ||
| while flow == 0: | ||
| await self.receive_events(timeout) | ||
| local_flow = self.state.local_flow_control_window(stream_id) | ||
| connection_flow = self.state.max_outbound_frame_size | ||
| flow = min(local_flow, connection_flow) | ||
| return flow |
There was a problem hiding this comment.
Some duplicated lines here - possible refactor?
while True:
local_flow = self.state.local_flow_control_window(stream_id)
connection_flow = self.state.max_outbound_frame_size
flow = min(local_flow, connection_flow)
if flow == 0:
await self.receive_events(timeout)
continue
return flowI can see that the current implementation really fits the docstring though, so not particularly urging about this.
There was a problem hiding this comment.
Yup, may well be worthwhile. I’ll get this in as-is for now, and can follow up.
* Enable mypy option `--warn-return-any` * Remove type ignore comment
This is my first draft onto refactoring the HTTP/2 implementation into something a bit more comprehensible and less brittle.
It's possible that the current implementation might be slow - I think we need to be listening in the background for WindowUpdated events while we're sending the data, rather than sending data until the flow control window hits zero, and then listening for a WindowUpdated event. I've not thought that all through enough.
Probably worth trying it out on a couple of sites to see if I've got that right or not.
It's a really nice, clear, clean-up tho. We actually have a proper isolated HTTP2Stream rather than just mangling all the state on a single class.