-
Notifications
You must be signed in to change notification settings - Fork 851
Reactivate active timeout enforcement #5824
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
|
Why don't you test other protocols (H1 on HTTPS, H2 and H3) ? Active / Inactive timeout is not only for cleartext H1. I know there is no way to test H3 currently, but for HTTPS and H2, you should be able to test them by adding 's' to the scheme and specifying |
|
Sure I'll add some more protocols. But at lest we are testing something :-). |
9d7aeb7 to
c91e618
Compare
|
Pushed another commit to augment test to exercise http/1.1 over https and http/2 over https as well. |
scw00
left a comment
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.
LGTM. It is more clear than before.
|
Thanks for adding tests.
True, but sometimes people unintentionally break something else when they fix something. I think we should add tests for all protocols even if we believe a fix doesn't break other protocols. |
c91e618 to
a1de689
Compare
|
Pushed up another commit to clean out the now useless conditional that @masaori335 pointed out. |
masaori335
left a comment
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.
Looks reasonable.
The event name was changed by apache#5824
The event name was changed by #5824
The event name was changed by apache#5824
The best that I can tell is that the active timeouts have not worked since 5.3.x with the dynamic active queue. Before that the active timeout was a scheduled event. Added tests to catch if this regresses again. Without the code change, the active_timeout.test.py would fail.
Stared at this for the better part of a day. Please let me know if I'm missing something in getting the active timeouts to trigger short of a code change.