Switched start_blocking_portal() to use daemon threads #750
Conversation
…readPoolExecutor ThreadPoolExecutor used daemon threads in Python 3.8 and non-daemon threads in 3.9 onwards. But daemon threads are essential for the use case where we want to have a "loitering" blocking portal which will only be shut down via an atexit hook.
|
I can confirm this appears to fix the hang I was observing 👍 |
|
Thanks for confirming! |
|
Possibly a random error which might be fixed by retrying the CI? |
Yup, randomly occurring Windows socket errors are among the hardest to figure out. |
src/anyio/from_thread.py
Outdated
| thread.start() | ||
| try: | ||
| portal = future.result() | ||
| except BaseException: | ||
| thread.join() | ||
| raise | ||
|
|
||
| if future.done(): | ||
| portal = future.result() | ||
| cancel_remaining_tasks = False | ||
| try: | ||
| yield portal | ||
| except BaseException: | ||
| cancel_remaining_tasks = True | ||
| raise | ||
| finally: | ||
| try: | ||
| portal.call(portal.stop, cancel_remaining_tasks) | ||
| except RuntimeError: | ||
| pass | ||
| cancel_remaining_tasks = False | ||
| try: | ||
| yield portal | ||
| except BaseException: | ||
| cancel_remaining_tasks = True | ||
| raise | ||
| finally: | ||
| try: | ||
| portal.call(portal.stop, cancel_remaining_tasks) | ||
| except RuntimeError: | ||
| pass | ||
|
|
||
| run_future.result() | ||
| thread.join() |
There was a problem hiding this comment.
I think removing the branching would make the mandatory-thread.join logic more clearly correct:
thread.start()
try:
cancel_remaining_tasks = False
portal = future.result()
try:
yield portal
except BaseException:
cancel_remaining_tasks = True
raise
finally:
try:
portal.call(portal.stop, cancel_remaining_tasks)
except RuntimeError:
pass
finally:
thread.join()(It would also protect against KeyboardInterrupts happening near the line cancel_remaining_tasks = False, although that is unlikely to ever matter, and there will still always be other KI races here.)
This is inconsequential though.
There was a problem hiding this comment.
I decided to apply this change.
tests/test_from_thread.py
Outdated
| async def raise_baseexception() -> None: | ||
| raise BaseException("fatal error") |
There was a problem hiding this comment.
| async def raise_baseexception() -> None: | |
| raise BaseException("fatal error") | |
| async def raise_baseexception() -> None: | |
| assert threading.current_thread().daemon | |
| raise BaseException("fatal error") |
|
One possibility to avoid breaking subinterpreters here could be to make this opt-in: |
I'm not too concerned about subinterpreters. I would've loved to add support for them, but they still don't have a stable Python API, and furthermore, once |
Changes
ThreadPoolExecutor used daemon threads in Python 3.8 and non-daemon threads in 3.9 onwards. But daemon threads are essential for the use case where we want to have a "loitering" blocking portal which will only be shut down via an atexit hook.
Related discussion: #743
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) added which would fail without your patchdocs/, in case of behavior changes or newfeatures)
docs/versionhistory.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**as the version.If, say, your patch fixes issue #123, the entry should look like this:
* Fix big bad boo-boo in task groups (#123 <https://github.com/agronholm/anyio/issues/123>_; PR by @yourgithubaccount)If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.