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

Use critical sections to protect I/O objects (in --disable-gil builds) #111965

Closed
Tracked by #108219
colesbury opened this issue Nov 10, 2023 · 7 comments
Closed
Tracked by #108219

Use critical sections to protect I/O objects (in --disable-gil builds) #111965

colesbury opened this issue Nov 10, 2023 · 7 comments
Labels
3.13 bugs and security fixes topic-free-threading type-feature A feature request or enhancement

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 10, 2023

Feature or enhancement

The I/O objects, like io.BufferedIOBase, io.TextIOWrapper, and io.StringIO have internal state that would not be thread-safe without the GIL.

We should be able to mostly use Argument Clinic's (AC) support for "critical sections" to guard methods on these objects. For operations that don't use AC, we can either convert them to use AC or write the Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION manually.

For context, here are the similar modifications in the nogil-3.12 fork, but the implementation in CPython 3.13 will be a bit different (no need for extra locks, use the syntax from #111903):

Linked PRs

@aisk
Copy link
Contributor

aisk commented Nov 17, 2023

The TextIOBase should be a base class and it's methods is just raise exceptions, so I think it dosen't need critical section protect. Is it a typo to TextIOWrapper? @colesbury

@colesbury
Copy link
Contributor Author

Yes, thanks, it should say TextIOWrapper. I've edited the issue.

@sobolevn
Copy link
Member

Sorry, what about _pyio?

@colesbury
Copy link
Contributor Author

@sobolevn, any specific concerns? I didn't see anything that looked like it required locking, but I may have missed something.

@sobolevn
Copy link
Member

No, no specific questions :)
I hoe that our new tests for threaded IO will catch any potential differences between two implementations 👍

@Mayuresh16
Copy link
Contributor

@colesbury Hi, I would like to give it a try for io.BufferedXXX implementation and will be happy to start my first open-source contribution with CPython based on my knowledge and understanding of the tools. 😅

@sobolevn
Copy link
Member

Looks like it is done! 🎉

aisk added a commit to aisk/cpython that referenced this issue Feb 11, 2024
aisk added a commit to aisk/cpython that referenced this issue Feb 11, 2024
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
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 topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants