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

Support coroutine exitfuncs for non-main loops #1368

Conversation

zmedico
Copy link
Member

@zmedico zmedico commented Aug 11, 2024

Since an API consumer can cause loops to be instantiated for non-main threads, support coroutine exitfuncs for each loop. The included Socks5ServerAtExitThreadedTestCase calls get_socks5_proxy from a non-main thread, and demonstrates that coroutine exitfuncs for the resulting non-main loop will reliably stop the socks5 proxy via atexit hook.

The _thread_weakrefs_atexit function will now make a temporary adjustment to _thread_weakrefs.loops so that a loop is associated with the current thread when it is closing. Also, the _get_running_loop function will now store weak references to all _AsyncioEventLoop instances it creates, since each has a _coroutine_exithandlers attribute that can be modified by atexit_register calls.

Bug: https://bugs.gentoo.org/937740

@zmedico zmedico marked this pull request as draft August 11, 2024 07:13
@zmedico zmedico force-pushed the bug_937740_coroutine_exitfuncs_for_non_main_loops branch 3 times, most recently from e9d1e67 to 1bddce1 Compare August 11, 2024 07:38
@zmedico zmedico marked this pull request as ready for review August 11, 2024 07:44
Since an API consumer can cause loops to be instantiated
for non-main threads, support coroutine exitfuncs for each
loop. The included Socks5ServerAtExitThreadedTestCase calls
get_socks5_proxy from a non-main thread, and demonstrates
that coroutine exitfuncs for the resulting non-main loop
will reliably stop the socks5 proxy via atexit hook.

The _thread_weakrefs_atexit function will now make a
temporary adjustment to _thread_weakrefs.loops so that a
loop is associated with the current thread when it is
closing. Also, the _get_running_loop function will now
store weak references to all _AsyncioEventLoop instances
it creates, since each has a _coroutine_exithandlers
attribute that can be modified by atexit_register calls.

Bug: https://bugs.gentoo.org/937740
Signed-off-by: Zac Medico <zmedico@gentoo.org>
@zmedico zmedico force-pushed the bug_937740_coroutine_exitfuncs_for_non_main_loops branch from 1bddce1 to cb0c09d Compare August 11, 2024 07:51
@floppym
Copy link
Contributor

floppym commented Aug 11, 2024

Sorry, but I don't have enough of a grasp on the async code in Portage to review this in any meaningful way.

if threaded:
t = threading.Thread(target=main)
t.start()
t.join()
Copy link
Member Author

@zmedico zmedico Aug 11, 2024

Choose a reason for hiding this comment

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

Note that functions like get_socks5_proxy do not currently have internal locking for thread safety, but we can safely neglect that here since this test case only has a single thread interacting with the portage API at a given time.

We can consider adding locking in the future, but for now our main goal should be to simply allow the portage APIs to be used in a single worker thread when the main thread is busy running a UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we use weak references to loops inside our asycio wrapper module, we can be sure that the loop won't be garbage collected before the socks5 proxy is stopped, since the socks5 proxy references the loop indirectly via ForkProcess and _GCProtector.

@zmedico
Copy link
Member Author

zmedico commented Aug 11, 2024

Sorry, but I don't have enough of a grasp on the async code in Portage to review this in any meaningful way.

Thanks anyway. By improving async support in atexit hooks, this pull request adds low-level support that has been missing since we adopted async code as the basis for the portage.process module in #1249.

@gentoo-bot gentoo-bot merged commit cb0c09d into gentoo:master Aug 13, 2024
13 checks passed
@zmedico zmedico deleted the bug_937740_coroutine_exitfuncs_for_non_main_loops branch August 13, 2024 21:03
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.

4 participants