hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 269 forks source link

Fix poll_capacity returning Ready(Some(0)) #627

Closed vadim-eg closed 2 years ago

vadim-eg commented 2 years ago

poll_capacity should never return Some(0) - because the caller has no way to be woken up when send capacity is available.

Check capacity and return Pending if it is zero. The wakeup code is already correctly triggers notification when capacity changes to non-zero.

seanmonstar commented 2 years ago

Thanks for the PR! I'm curious, do you know if we have a unit test that exercises?

vadim-eg commented 2 years ago

Thanks for the PR! I'm curious, do you know if we have a unit test that exercises?

It looks like poll_capacity_after_send_data_and_reserve[_with_max_send_buffer_size] do attempt to test the condition - frankly I haven't figured out yet the exact conditions triggering it. In our case Some(0) happens fairly sporadically usually involving multiple streams doing bidirectional io. If I find some cycles I'll try to take a closer look. Thanks!

seanmonstar commented 2 years ago

It looks like in CI, this causes a test to hang:

test reserved_capacity_assigned_in_multi_window_updates has been running for over 60 seconds

vadim-eg commented 2 years ago

It looks like in CI, this causes a test to hang:

test reserved_capacity_assigned_in_multi_window_updates has been running for over 60 seconds

@seanmonstar Yes, I'll try to see what is going on.

vadim-eg commented 2 years ago

@seanmonstar I think I got an explanation on the original issue - Poll(Ready(Some(0))). It appears to be a concurrency/timing issue - it can happen when wakeup is called from one thread while the outer is sending data on a stream - e.g. -

vadim-eg commented 2 years ago

I think I have an idea what is going on I'll open an issue and take it there