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

Fix nursery crashing during a guest run when the host loop calls start_soon(needs_sniffio) #2462

Merged

Conversation

gschaffner
Copy link
Member

@gschaffner gschaffner commented Nov 5, 2022

Closes #2183.

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #2462 (ee0bf40) into master (d6454d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2462   +/-   ##
=======================================
  Coverage   92.45%   92.45%           
=======================================
  Files         118      118           
  Lines       16342    16351    +9     
  Branches     3155     3156    +1     
=======================================
+ Hits        15109    15118    +9     
  Misses       1104     1104           
  Partials      129      129           
Impacted Files Coverage Δ
trio/_threads.py 100.00% <ø> (ø)
trio/_core/_run.py 98.49% <100.00%> (-0.01%) ⬇️
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)

@gschaffner gschaffner force-pushed the fix-sniffio-guest-run-start_soon branch from 32f8c0c to e45cec2 Compare November 5, 2022 07:13
@gschaffner gschaffner changed the title Fix start_soon(needs_sniffio) failing when called by the host loop during a guest run Fix nursery crashing when the host loop calls start_soon(needs_sniffio) during a guest run Nov 5, 2022
@gschaffner gschaffner changed the title Fix nursery crashing when the host loop calls start_soon(needs_sniffio) during a guest run Fix nursery crashing during a guest run when the host loop calls start_soon(needs_sniffio) Nov 5, 2022
@gschaffner gschaffner force-pushed the fix-sniffio-guest-run-start_soon branch from e45cec2 to d826355 Compare November 5, 2022 07:17
@gschaffner gschaffner force-pushed the fix-sniffio-guest-run-start_soon branch from 441514d to 44fe095 Compare November 5, 2022 07:36
@gschaffner
Copy link
Member Author

I just force-pushed what should be a fix for the bug in the test.

@gschaffner gschaffner force-pushed the fix-sniffio-guest-run-start_soon branch from 44fe095 to 7ecfbd2 Compare November 9, 2022 07:38
@gschaffner
Copy link
Member Author

I've just fixed the host_uses_signal_set_wakeup_fd-related test hangs that were occurring on Windows when asyncio.ProactorEventLoop from CPython >= 3.8 was used.

(Curiously, the test only hung forever when using a >= 3.8 ProactorEventLoop and running it from pytest. It ran fine on ProactorEventLoop when called like python -c 'from trio._core.tests.test_guest_mode import test_guest_mode_host_calls_start_soon_sniffio; test_guest_mode_host_calls_start_soon_sniffio()'.)

@gschaffner gschaffner force-pushed the fix-sniffio-guest-run-start_soon branch from 0802b9b to 977a71c Compare December 20, 2022 10:06
@gschaffner
Copy link
Member Author

rebased onto master and updated the newsfragment issue number.

@gschaffner
Copy link
Member Author

gschaffner commented Dec 20, 2022

the CI failure on pypy3.8 nightly on Ubuntu appears to be unrelated to this PR and may just be a flaky failure. the new test that this PR adds did pass on the pypy3.8 nightly on Ubuntu CI. the failure appears to be due to a segfault that occurred while pytest was handling test_dtls.py::test_smoke[ipv6]'s timeout. (note: the same timeout (without a segfault) also occurred on the latest pypy3.8 nightly on Ubuntu CI run on master (https://github.com/python-trio/trio/actions/runs/3684296901/jobs/6233869817#step:5:1073).)

the failure of the new test on Windows+Komodia LSP, however, was occurring before the rebase and is still something that is pretty beyond me—I would be much appreciative if someone familiar with Komodia LSP could help look into it.

@richardsheridan richardsheridan force-pushed the fix-sniffio-guest-run-start_soon branch from 2e05760 to 9e9582e Compare March 6, 2023 00:03
@richardsheridan
Copy link
Contributor

The new commit I pushed merges #2574 into this PR, in essentially the state that #2574 would be by applying your review comments to it. Using this roundabout method has the advantage of acknowledging your contribution and inviting you to join the python-trio org! If you acknowledge that you are ok with this, I'll bring it all up to date with master and merge.

@gschaffner
Copy link
Member Author

this looks great to me, thanks @richardsheridan!

also, for what it's worth, i am already a member of the Trio github org, but i still appreciate that this PR acknowledges both my and oremanj's contributions here :)

(i'm also on the trio gitter—not sure what caused trouble when you tried searching for me there!)

@@ -0,0 +1 @@
Fix :func:`sniffio.current_async_library` when Trio tasks are spawned from a non-Trio context (such as when using trio-asyncio). Previously, a regular Trio task would inherit the non-Trio library name, and spawning a system task would cause the non-Trio caller to start thinking it was Trio.
Copy link
Member Author

Choose a reason for hiding this comment

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

it might make sense to overwrite 2183.bugfix.rst with this new message instead of keeping both. my message in 2183.bugfix.rst just describes a specific case of oremanj's message here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardsheridan it looks like you did git mv newsfragments/{2574,2462}.bugfix.rst, meaning there are still two redundant changelog entries—was that your intention? to clarify, i meant to suggest git mv -f newsfragments/{2574,2183}.bugfix.rst

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! I think the commit message phrasing fits better if the link points here to the fix. I'll push a clean renumber commit when CI is working again.

@richardsheridan
Copy link
Contributor

I also added a test for your edge case of a function returning a coroutine!

@richardsheridan richardsheridan force-pushed the fix-sniffio-guest-run-start_soon branch from 6152019 to ee0bf40 Compare March 8, 2023 23:52
@richardsheridan richardsheridan merged commit 1b50186 into python-trio:master Mar 9, 2023
29 checks passed
@gschaffner gschaffner deleted the fix-sniffio-guest-run-start_soon branch March 9, 2023 00:19
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.

trio's guest mode breaks sniffio.current_async_library in certain cases
3 participants