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

asyncio thread-safety issues in the free-threaded build #120974

Closed
colesbury opened this issue Jun 24, 2024 · 2 comments
Closed

asyncio thread-safety issues in the free-threaded build #120974

colesbury opened this issue Jun 24, 2024 · 2 comments
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-asyncio topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Jun 24, 2024

Bug report

  1. fi_freelist isn't thread-safe (move to pycore_freelist.h and follow that pattern)
  2. enter_task, leave_task, and swap_current_task aren't thread-safe due to shared state->current_tasks and borrowed references.
  3. register_task and unregister_task aren't thread-safe due to shared state->asyncio_tasks linked list
  4. _asyncio_all_tasks_impl isn't thread-safe due the the asyncio_tasks linked list.

For 2, 3, and 4, we can consider using critical sections to protect the accesses to state->current_tasks and state->asyncio_tasks.

Longer term, moving data to per-loop will probably help with multi-threaded scaling.

Linked PRs

@colesbury colesbury added type-bug An unexpected behavior, bug, or error topic-asyncio 3.13 bugs and security fixes topic-free-threading 3.14 new features, bugs and security fixes labels Jun 24, 2024
@github-project-automation github-project-automation bot moved this to Todo in asyncio Jun 24, 2024
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Jun 25, 2024

I think that the pure Python fallback isn't thread safe either, FWIW I think there are more thread safety issues than these, asyncio code was written with assumption of GIL.

@corona10
Copy link
Member

asyncio code was written with assumption of GIL.

Yeah, but for landing PEP 703, we are trying to change modules, not depending on the GIL, even if we need to re-write some parts of the implementation with a conditional flag. (If the new written part requires some performance overhead)

colesbury added a commit to colesbury/cpython that referenced this issue Jul 22, 2024
This refactors asyncio to use the common freelist helper functions and
macros. As a side effect, the freelist for _asyncio.Future is now
re-enabled in the free-threaded build.
colesbury added a commit to colesbury/cpython that referenced this issue Jul 22, 2024
…d build

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
colesbury added a commit to colesbury/cpython that referenced this issue Jul 22, 2024
…d build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
kumaraditya303 pushed a commit that referenced this issue Jul 23, 2024
#122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 23, 2024
…d build (pythonGH-122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
(cherry picked from commit 47847aa)

Co-authored-by: Sam Gross <[email protected]>
kumaraditya303 pushed a commit that referenced this issue Jul 23, 2024
…ed build (GH-122138) (#122152)

gh-120974: Make _asyncio._enter_task atomic in the free-threaded build (GH-122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
(cherry picked from commit 47847aa)

Co-authored-by: Sam Gross <[email protected]>
kumaraditya303 pushed a commit that referenced this issue Jul 23, 2024
This refactors asyncio to use the common freelist helper functions and
macros. As a side effect, the freelist for _asyncio.Future is now
re-enabled in the free-threaded build.
kumaraditya303 pushed a commit that referenced this issue Jul 23, 2024
#122139)

* gh-120974: Make _asyncio._leave_task atomic in the free-threaded build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 23, 2024
…d build (pythonGH-122139)

* pythongh-120974: Make _asyncio._leave_task atomic in the free-threaded build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
(cherry picked from commit a15fede)

Co-authored-by: Sam Gross <[email protected]>
kumaraditya303 pushed a commit that referenced this issue Jul 23, 2024
…ed build (GH-122139) (#122186)

gh-120974: Make _asyncio._leave_task atomic in the free-threaded build (GH-122139)

* gh-120974: Make _asyncio._leave_task atomic in the free-threaded build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
(cherry picked from commit a15fede)

Co-authored-by: Sam Gross <[email protected]>
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
…d build (python#122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
This refactors asyncio to use the common freelist helper functions and
macros. As a side effect, the freelist for _asyncio.Future is now
re-enabled in the free-threaded build.
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
…d build (python#122139)

* pythongh-120974: Make _asyncio._leave_task atomic in the free-threaded build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
…d build (python#122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
This refactors asyncio to use the common freelist helper functions and
macros. As a side effect, the freelist for _asyncio.Future is now
re-enabled in the free-threaded build.
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
…d build (python#122139)

* pythongh-120974: Make _asyncio._leave_task atomic in the free-threaded build

Update `_PyDict_DelItemIf` to allow for an argument to be passed to the
predicate.
colesbury added a commit to colesbury/cpython that referenced this issue Jul 26, 2024
…ed build

Use a critical section around the modifications to `current_tasks`.
colesbury added a commit to colesbury/cpython that referenced this issue Jul 26, 2024
…ed build

Use a critical section around the modifications to `current_tasks`.
kumaraditya303 pushed a commit that referenced this issue Aug 2, 2024
…ld (#122317)

* gh-120974: Make asyncio `swap_current_task` safe in free-threaded build
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 2, 2024
…ed build (pythonGH-122317)

* pythongh-120974: Make asyncio `swap_current_task` safe in free-threaded build
(cherry picked from commit b5e6fb3)

Co-authored-by: Sam Gross <[email protected]>
kumaraditya303 pushed a commit that referenced this issue Aug 2, 2024
…ded build (GH-122317) (#122612)

gh-120974: Make asyncio `swap_current_task` safe in free-threaded build (GH-122317)

* gh-120974: Make asyncio `swap_current_task` safe in free-threaded build
(cherry picked from commit b5e6fb3)

Co-authored-by: Sam Gross <[email protected]>
brandtbucher pushed a commit to brandtbucher/cpython that referenced this issue Aug 7, 2024
…ed build (python#122317)

* pythongh-120974: Make asyncio `swap_current_task` safe in free-threaded build
kumaraditya303 added a commit that referenced this issue Aug 11, 2024
Make `_asyncio.all_tasks` thread safe, also changes state lock to use critical section.
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Aug 11, 2024
blhsing pushed a commit to blhsing/cpython that referenced this issue Aug 22, 2024
…ed build (python#122317)

* pythongh-120974: Make asyncio `swap_current_task` safe in free-threaded build
blhsing pushed a commit to blhsing/cpython that referenced this issue Aug 22, 2024
Make `_asyncio.all_tasks` thread safe, also changes state lock to use critical section.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-asyncio topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

3 participants