-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(http1): poll_loop writes when ready #3952
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
Conversation
4862c20 to
b806fb9
Compare
b806fb9 to
f2aa734
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! This loop can be tricky, it was initially added to improve performance when multiple messages have been batched together. But the loop has a limit, since otherwise it could starve all other futures. (Tokio's cooperative budget can help here, but hyper doesn't assume it's Tokio.)
Looking close, I appreciate you cleaning up the mistake in here: since we're essentially "selecting" over read and write, we need to make sure both sides either registered a waker, or that we will poll again with the yield_once util. Thanks for the attention!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my only comment for this fix is that if possible, a simpler unit test would be easier to understand and maintain in the future. Can that be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little difficult to simplify because I needed to create a future that would deterministically trigger this edge case.
What's "weird" about this future mock is that it can return Poll::Pending once, but then reach completion on a future poll without the waker being woken, so I don't think it ever happens in Tokio for example but it does happen (probabilistically) with my actual futures that this is modeling. And as far as I know it's no violation of the Futures contract to do so.
Anyway, I'm not sure I can create the situation with less or more simple code. That being said, maybe a fixture like this is generalizable for the project? Creating a "connection" that's purely channel based software could allow other edge cases to be explored. Partial reads and writes or different sequence of readiness.
All that being said, I'll still take another critical pass at what I've done here next week and I'll see what I can do to simplify.
It's an interesting loop and I think with this edge case handled it works well. It would be nice to expose the 16 as a config (as noted in the comments) and maybe I'll PR that later.
One thing that I considered while looking at it is that maybe doing some tricks with custom wakers could be nice? Similar to FuturesUnordered, the waker could indicate who is actually ready for polling. The juice is probably not worth the squeeze (or perhaps even an efficiency loss with all the bookkeeping?) as I know FuturesUnordered is intended for some level of scale and three is probably not it.
Yea, hm, if it's a complicated edge case, perhaps that's fine as-is, then.
Uh oh!
There was an error while loading. Please reload this page.
I ran into some lockups running hyper with some custom futures. If one of my futures is ready when polled, the waker is never signaled and I think this uncovered a logical issue with the http1 poll_loop. That it to say, if the poll_write is demonstrating readiness to write and the connection has readiness to write, we should do that or else hyper may stall.
I built a pathological example "ready_stream.rs" - you can run it from this branch with:
RUSTFLAGS='--cfg hyper_unstable_tracing' cargo test --test ready_stream --features full,tracing.I provided a proposed patch for the poll_loop, but I'm open to other angles on this.