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.

Sign up for GitHub

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

Jump to bottom

gh-128002: fix many thread safety issues in asyncio #128147

Merged
merged 26 commits into from
Jan 4, 2025
Merged

gh-128002: fix many thread safety issues in asyncio #128147

merged 26 commits into from
Jan 4, 2025

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Dec 21, 2024 •
edited
Loading

Changes:

  • Makes _asyncio.Task and _asyncio.Future thread-safe by adding critical sections
  • Add assertions to check for thread safety checking locking of object by critical sections in internal functions
  • Make _asyncio.all_tasks thread safe when eager tasks are used
  • Change asyncio state lock to a mutex rather than critical section to fix re-entrancy crash, not really sure about this one
  • Add a thread safety test

@bedevere-app bedevere-app bot mentioned this pull request Dec 21, 2024
Open
9 tasks
@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Dec 31, 2024 •
edited
Loading

@colesbury While running the tests locally on forever mode I see frequent tsan warnings and rarely some crashes in exceptions.c arising from things like setting traceback on a exception, should I add those to the suppressions list?

colesbury
colesbury reviewed Jan 2, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

I'm still reading through the other locking changes, but a comment on all_tasks below:

Modules/_asynciomodule.c Outdated Show resolved Hide resolved
Modules/_asynciomodule.c Outdated Show resolved Hide resolved
@kumaraditya303 kumaraditya303 mentioned this pull request Jan 2, 2025
Open
@kumaraditya303
Copy link
Contributor Author

Filed #128421 for existing data races in traceback and frame inspection.

colesbury
colesbury reviewed Jan 2, 2025
View reviewed changes
Tools/tsan/suppressions_free_threading.txt Outdated Show resolved Hide resolved
Modules/_asynciomodule.c Outdated Show resolved Hide resolved
Modules/_asynciomodule.c Outdated Show resolved Hide resolved
colesbury
colesbury reviewed Jan 2, 2025
View reviewed changes
Modules/_asynciomodule.c Outdated Show resolved Hide resolved
colesbury
colesbury approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM with two comments below

Modules/_asynciomodule.c Show resolved Hide resolved
Tools/tsan/suppressions_free_threading.txt Outdated Show resolved Hide resolved
@kumaraditya303 kumaraditya303 merged commit 513a4ef into python:main Jan 4, 2025
39 checks passed
@kumaraditya303 kumaraditya303 deleted the asyncio-tsafe branch January 4, 2025 08:48
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull request Jan 4, 2025
@kumaraditya303 @WolframAlph
pythongh-128002: fix many thread safety issues in asyncio (python#128147
bdfe2ca 
)

* Makes `_asyncio.Task` and `_asyncio.Future` thread-safe by adding critical sections
* Add assertions to check for thread safety checking locking of object by critical sections in internal functions
* Make `_asyncio.all_tasks` thread safe when eager tasks are used
* Add a thread safety test
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@kumaraditya303 @srinivasreddy
pythongh-128002: fix many thread safety issues in asyncio (python#128147
4a1dabe 
)

* Makes `_asyncio.Task` and `_asyncio.Future` thread-safe by adding critical sections
* Add assertions to check for thread safety checking locking of object by critical sections in internal functions
* Make `_asyncio.all_tasks` thread safe when eager tasks are used
* Add a thread safety test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury colesbury approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

Assignees
No one assigned
Labels
skip news topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kumaraditya303 @colesbury