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

Allow start_clock() to start the very first system task; make MockClock do so #1579

Closed
wants to merge 3 commits into from

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented Jun 7, 2020

Preparatory to #1521 in which the order will do anything. As discussed in #265, the autojumper should be the very last thing to get shut down, even after the run_sync_soon task.

This requires some shenanigans because start_clock() runs before the first step of init. We effectively execute the system nursery __aenter__ outside of async context, so as to have a system nursery when start_clock() runs.

@oremanj oremanj requested a review from njsmith June 7, 2020 09:51
@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #1579 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1579   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         111      111           
  Lines       13866    13866           
  Branches     1059     1059           
=======================================
  Hits        13824    13824           
  Misses         27       27           
  Partials       15       15           
Impacted Files Coverage Δ
trio/_core/_run.py 99.76% <100.00%> (+<0.01%) ⬆️
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)
trio/testing/_mock_clock.py 100.00% <100.00%> (ø)

@njsmith
Copy link
Member

njsmith commented Jun 7, 2020

What do you think of the idea discussed in the last few paragraphs of this comment, of making MockClock use an instrument instead of a task? #239 (comment)

@oremanj
Copy link
Member Author

oremanj commented Jun 8, 2020

Interesting! I had rather glossed over that issue before, but have now gone back and read it in detail.

I think the instrument-based MockClock is going to be challenging to make work in all edge cases without more instrumentation hooks churn than I'm up for right now. (This issue is to unblock #1521, which together with #1543 unblocks #1564 and #1537... the yak stack depth is getting intimidating!) I commented on #239 with some hazards I saw when I sat down to try it.

I certainly don't mind removing the unusual system_nursery initialization in this PR if MockClock later changes to make it unnecessary.

@njsmith
Copy link
Member

njsmith commented Jun 9, 2020

I'm intrigued by the idea in this PR to make our bootstrap code start by creating a nursery, instead of starting by creating a task. It's not something I've really thought about before.

As far as fixing MockClock goes though, I think #1588 is probably easier to reason about than this PR, and also sets us up better for future features like chaos scheduling?

@oremanj
Copy link
Member Author

oremanj commented Jun 9, 2020

Closing this in favor of #1588. If we later need the system nursery to exist earlier for something else, that's what version control is for!

@oremanj oremanj closed this Jun 9, 2020
@oremanj oremanj deleted the run-autojumper-always branch June 9, 2020 10:16
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