fix(http1): poll_loop writes when ready

[lthiery]

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.

[seanmonstar]

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!

[seanmonstar]

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?

[lthiery]

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.

[lthiery]

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.

[seanmonstar]

Yea, hm, if it's a complicated edge case, perhaps that's fine as-is, then.