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

_base.py: fix double acquire by TrioExecuto, issue #143 #144

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

axiomiety
Copy link
Contributor

@axiomiety axiomiety commented Apr 8, 2024

Fixes #143

Ran pytest on both master and the branch with the fix - output is the same (there are existing failures):

  • on executor-double-acquire-fix
=============================================== short test summary info ================================================
FAILED tests/interop/test_calls.py::TestCalls::test_trio_asyncio_cancel_direct - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_keyboard_interrupt_teardown - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_cancel_loop[True] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_run_trio_task_errors - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_trio_asyncio.py::test_cancel_loop_with_tasks[True-False] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_trio_asyncio.py::test_cancel_loop_with_tasks[True-True] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
=========================== 6 failed, 2504 passed, 62 skipped, 4 xfailed in 71.12s (0:01:11) ===========================
  • on master
================================================ short test summary info ================================================
FAILED tests/interop/test_calls.py::TestCalls::test_trio_asyncio_cancel_direct - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_keyboard_interrupt_teardown - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_cancel_loop[True] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_misc.py::test_run_trio_task_errors - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_trio_asyncio.py::test_cancel_loop_with_tasks[True-False] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/test_trio_asyncio.py::test_cancel_loop_with_tasks[True-True] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
=========================== 6 failed, 2504 passed, 62 skipped, 4 xfailed in 71.33s (0:01:11) ============================

@oremanj
Copy link
Member

oremanj commented Apr 17, 2024

Please add a newsfragment for this change, as well as a test that would have failed before the fix and now doesn't. Once those are set, I should be able to merge this.

@axiomiety axiomiety force-pushed the executor-double-acquire-fix branch 6 times, most recently from 368ad2b to 57851d4 Compare April 18, 2024 07:47
Copy link
Member

@oremanj oremanj left a comment

Choose a reason for hiding this comment

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

Thanks for the test!

@@ -0,0 +1 @@
Fix double-acquire by in the executor that could lead to deadlock
Copy link
Member

Choose a reason for hiding this comment

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

This will show up in the release notes; please try a little harder to be detailed and clear. One possibility is shown.

Suggested change
Fix double-acquire by in the executor that could lead to deadlock
`TrioExecutor` now acquires just one token from its `~trio.CapacityLimiter` for each
worker thread that it runs, rather than two. The double-acquisition previously could
cause a deadlock when trying to run a large number of tasks simultaneously using
:meth:`asyncio.loop.run_in_executor`.

@axiomiety axiomiety force-pushed the executor-double-acquire-fix branch from 57851d4 to 9d6103f Compare April 18, 2024 10:00
@oremanj oremanj merged commit ffbd11c into python-trio:master Apr 18, 2024
25 checks passed
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.

TrioExecutor double-acquires the limiter, which can deadlock
2 participants