Skip to content
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

use autojump_threshold in test_handshake_over_terrible_network #2603

Merged
merged 2 commits into from
May 4, 2023

Conversation

richardsheridan
Copy link
Contributor

observation: any autojump threshold over 0 takes MUCH longer than 0
hypothesis: 10 second timeout cscope at the end of test is spuriously cancelling attempts on slow machines
experiment: set a minimal threshold and scale back # of handshakes to finish in a reasonable time, check if CI timeouts are avoided

observation: any autojump threshold over 0 takes MUCH longer than 0
hypothesis: 10 second timeout cscope at the end of test is spuriously cancelling attempts on slow machines
experiment: set a minimal threshold and scale back # of handshakes to finish in a reasonable time, check if CI timeouts are avoided
@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

Merging #2603 (4aace6d) into master (e45c9d5) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2603   +/-   ##
=======================================
  Coverage   92.45%   92.45%           
=======================================
  Files         118      118           
  Lines       16352    16352           
  Branches     3156     3156           
=======================================
  Hits        15119    15119           
  Misses       1104     1104           
  Partials      129      129           
Impacted Files Coverage Δ
trio/tests/test_dtls.py 99.79% <100.00%> (ø)

@richardsheridan
Copy link
Contributor Author

@A5rocks I think this puts the cherry on top of #2536 what do you think? I might even try a version that makes HANDSHAKES consistent across platforms again.

@A5rocks
Copy link
Contributor

A5rocks commented Mar 11, 2023

I'm not sure I agree with the reasoning in the description of the PR (the 10 second cancel scope should solely be skipping sends?). I think I agree with that changes though, just the description needs to be rethought!

I would make HANDSHAKES consistent across platforms if it works.

@richardsheridan
Copy link
Contributor Author

I suppose I should put a fuller explanation here in the PR. I just learned about this stuff today as well after all!

The autojump_clock pytest fixture installs a MockClock that transforms any timeouts/fail_afters/deadlines into immediate wakeups. It is one step removed from actually calling cancel_scope.cancel() right before your task goes to sleep. The interaction of this tool with this test has the effect of making magnitude of a timeout argument irrelevant.

My thinking is that the send is effectively "nonblocking" and immediately reschedules itself, whereas the receive has to wait long enough for the system (kernel? python?) to get around to handling the sent payload. This handling process may be just slow enough on an overloaded CI machine to outlast one cycle of the run loop that applies for the default setting, but not the 1 ms autojump_threshold (or for that matter a 1 microsecond threshold that I tested locally).

Finally, it does seem to work, I've rerun CI on 55abf37 4 times and never had a pypy timeout failure. Take another look after I unify HANDSHAKES, so can merge this and stabilize CI! we can always fix any wording or explanatory comments in a separate issue.

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

That does seem like a pretty important issue! Alright, I'm on board. I'd rather wait for confirmation on whether this autojump + cancel scope issue might impact elsewhere (from maybe njs?), though that's not too important.

@A5rocks
Copy link
Contributor

A5rocks commented May 4, 2023

I shouldn't have left this out to dry for so long, sorry!

@A5rocks A5rocks merged commit 6f815cd into python-trio:master May 4, 2023
@richardsheridan richardsheridan deleted the dtls_autojump_threshold branch May 22, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants