-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
fix: streams awaiting capacity lockout #730
Conversation
This PR changes the the assign-capacity queue to prioritize streams that are send-ready. This is necessary to prevent a lockout when streams aren't able to proceed while waiting for connection capacity, but there is none.
Thank you so so much! I'm wondering, would it be complicated to make a unit test that exercises this? Or maybe one of the reporters would like to try the patch and confirm the problem is gone. |
I tested it on the minimal repro sample provided in the issue, and it seems to be working fine. It also fixes this issue:
I'm currently trying to see if we can make this happen but haven't had the time yet. Hence, the draft status Also, is there any benchmark for hyper at the moment? @seanmonstar |
That's fantastic! For benchmarks, the end_to_end ones are likely best. You can use a |
Can this change result in a situation where new streams that are not Update: Looking at this in more detail I don't think the scenario in the question above can happen. It is still not clear to me what the root cause of the problem is. Do we ever attempt to assign capacity to streams that are not open yet? |
What's happening is that the so here's an example:
I've added a test to demonstrate an extreme case of this: e13adae While 7dec3c2 does prevent a lockout, f1f99e0 is a more complete one @seanmonstar. Haven't had the chance to formally benchmark it (hyper benchmark is broken in my machine somehow), but against the go server it seems fine. |
Thanks for the explanation. I think the changes in f1f99e0 are good. |
I can run the benches. I realized the http2 chunked benchmarks could be re-enabled, once that's done I'll compare them. |
Alright, here's the benchmarks, ran 3 times each: master
patch
|
Looking through the results, the differences look like just noise. Maybe there's a workload that matters that the benchmarks don't catch, but 🤷 . If you concur, we can merge this! |
I didn't work on this, but it does look like noise to me. Some of them are highly variable in both |
I noticed that too. I think the issue is that the benchmarks are going over localhost, which means adaptive window which tries to detect bandwidth will have a horrible time. A cool project to improve the adaptive window option would be to setup some tests using turmoil where we can simulate latency. |
Thank you so much @dswij, excellent debugging work here ❤️ |
Shipped as v0.4.1 :) |
This PR changes the the assign-capacity queue to prioritize streams that are send-ready. This is necessary to prevent a lockout when streams aren't able to proceed while waiting for connection capacity, but there is none. Closes hyperium/hyper#3338
This PR changes the the assign-capacity queue to prioritize streams that are send-ready. This is necessary to prevent a lockout when streams aren't able to proceed while waiting for connection capacity, but there is none. Closes hyperium/hyper#3338 Co-authored-by: dswij <dharmasw@outlook.com>
Fixes hyperium/hyper#3338
This PR changes the the assign-capacity queue to prioritize streams that are send-ready.
This is necessary to prevent a lockout when streams aren't able to proceed while waiting for connection capacity, but there is none.