Skip to content

Conversation

@shinrich
Copy link
Member

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.

@shinrich shinrich added this to the 9.0.0 milestone Aug 13, 2019
@shinrich shinrich self-assigned this Aug 13, 2019
@maskit
Copy link
Member

maskit commented Aug 14, 2019

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 --http2 option to curl.

@shinrich
Copy link
Member Author

Sure I'll add some more protocols. But at lest we are testing something :-).

@shinrich shinrich force-pushed the fix-active-timeout branch from 9d7aeb7 to c91e618 Compare August 14, 2019 19:52
@shinrich
Copy link
Member Author

Pushed another commit to augment test to exercise http/1.1 over https and http/2 over https as well.

scw00
scw00 previously approved these changes Aug 15, 2019
Copy link
Member

@scw00 scw00 left a 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.

@maskit
Copy link
Member

maskit commented Aug 15, 2019

Thanks for adding tests.

But at lest we are testing something :-).

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.

@shinrich
Copy link
Member Author

Pushed up another commit to clean out the now useless conditional that @masaori335 pointed out.

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

@shinrich shinrich merged commit 3e039a2 into apache:master Aug 18, 2019
maskit added a commit to maskit/trafficserver that referenced this pull request Aug 19, 2019
The event name was changed by apache#5824
maskit added a commit that referenced this pull request Aug 21, 2019
The event name was changed by #5824
ema pushed a commit to ema/trafficserver that referenced this pull request Oct 24, 2019
The event name was changed by apache#5824
@zwoop zwoop modified the milestones: 9.0.0, 8.1.0 Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants