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

Ensure guest run is initialized when start_guest_run() returns #2696

Merged

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented Jul 11, 2023

Run a few ticks of the guest run synchronously within start_guest_run, so that global Trio operations such as current_time() and spawn_system_task() work as soon as start_guest_run returns (rather than at some indefinite point in the near future).

Motivating use case: allowing trio/asyncio integration beginning from asyncio mode to start up a Trio run and create a backing Trio task for an asyncio task that's about to enter Trio mode, all from a sync-colored context. The reverse (starting an asyncio guest run synchronously) is already supported by aioguest and is necessary to support sync-colored wrappers around asyncio-flavored async functions; that idiom is less common for Trio-flavored code, but it's still convenient to be able to treat both directions of transition in the same way, and it seems likely this PR could simplify other guest-mode wrappers as well.

@oremanj oremanj requested a review from A5rocks July 11, 2023 10:08
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #2696 (cc2d74a) into master (d3255a0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2696   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files         113      113           
  Lines       16851    16885   +34     
  Branches     3032     3036    +4     
=======================================
+ Hits        16673    16707   +34     
  Misses        123      123           
  Partials       55       55           
Files Changed Coverage Δ
trio/_core/_run.py 99.44% <100.00%> (+<0.01%) ⬆️
trio/_core/_tests/test_guest_mode.py 99.68% <100.00%> (+0.02%) ⬆️

@oremanj oremanj requested a review from njsmith July 11, 2023 10:17
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.

This looks sane and makes sense to me (though I have a couple bits I don't get, like next_send being different on Windows and non-Windows), but I really don't know how guest runs work or anything like that.

Probably wait for someone else to review!

@oremanj
Copy link
Member Author

oremanj commented Jul 14, 2023

next_send is supposed to have a type that matches the return value of IOManager.get_events(). On Linux it's a list of events, while on Windows it's a count of events; the difference owes to low-level API arcana. We don't bother actually checking for I/O during these first few ticks because there shouldn't be any tasks that need it yet. I'll add a comment.

@A5rocks
Copy link
Contributor

A5rocks commented Aug 24, 2023

Well, hopefully someone with knowledge reviews this + #2700 in the near future! (Just bumping this :P )

@richardsheridan
Copy link
Contributor

This PR has a typing "problem" in that it correctly sends None in the first pass through the loop, but the typing annotations don't seem to understand that that is required. I don't know enough about typing to fix that but since there are other typing problems on master, I'm just merging to get this fix in and bring #2700 up to date as well.

@richardsheridan richardsheridan merged commit 55db083 into python-trio:master Sep 3, 2023
26 of 27 checks passed
richardsheridan added a commit that referenced this pull request Sep 3, 2023
Fix type errors introduced when #2696 and #2700 were merged
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.

3 participants