GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-116738: Make `_codecs` module thread-safe by swtaarrs · Pull Request #117530 · python/cpython

Conversation

@swtaarrs

@swtaarrs swtaarrs commented Apr 4, 2024

Copy link
Copy Markdown
Member

The module itself is a thin wrapper around calls to functions in Python/codecs.c, so that's where the meaningful changes happened:

  • Move codecs-related state that lives on PyInterpreterState to a struct declared in pycore_codecs.h.

  • In free-threaded builds, add two mutexes to codecs_state, one to synchronize operations on search_path, and one to synchronize initialization.

  • _PyCodecRegistry_Init() is now _PyCodecRegistry_EnsureInit(), and is called unconditionally to ensure initialization, rather than only when initialization is needed. When it returns 0 (as opposed to -1 to indicate an error), the PyObject * members of codecs_state can be read without further synchronization.

  • Operations that mutate codecs.search_path must be performed while holding codecs.search_path_mutex. This allows PyCodec_Unregister() to search for and delete a specific item from the list. Because search_path_mutex is used as a normal mutex and not a critical section, we must be extremely careful with operations called while holding it.

  • Issue: Audit all built-in modules for thread safety #116738

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@swtaarrs
Make _codecs module thread-safe
574e48c
@swtaarrs swtaarrs added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading labels Apr 4, 2024
@swtaarrs
swtaarrs requested a review from colesbury April 4, 2024 00:27
@bedevere-app bedevere-app Bot mentioned this pull request Apr 4, 2024
Audit all built-in modules for thread safety #116738
Open
@swtaarrs
Try to fix check-c-globals
d8bcdd8
@swtaarrs
swtaarrs marked this pull request as ready for review April 4, 2024 17:49
@swtaarrs
swtaarrs requested a review from ericsnowcurrently as a code owner April 4, 2024 17:49
@bedevere-app bedevere-app Bot added the awaiting review label Apr 4, 2024
colesbury
colesbury reviewed Apr 10, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks pretty good. Two comments below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/codecs.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/codecs.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

swtaarrs added 2 commits April 14, 2024 18:03
@swtaarrs
Initialize codec registry eagerly, greatly simplifying things
8076032
@swtaarrs
Merge remote-tracking branch 'upstream/main' into cpython-_codecs-module
137cf27
colesbury
colesbury reviewed Apr 15, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks good. A few comments below.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/codecs.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@swtaarrs
Final review comments, and add missing newline after _PyCodec_InitReg…
4d5bffc
…istry's return type
@swtaarrs

swtaarrs commented Apr 26, 2024

Copy link
Copy Markdown
Member Author

This should be ready to go as of my last commit - let me know if any other changes are needed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

swtaarrs added 2 commits April 26, 2024 12:44
@swtaarrs
Merge remote-tracking branch 'upstream/main' into cpython-_codecs-module
dea7cf5
@swtaarrs
Merge remote-tracking branch 'upstream/main' into cpython-_codecs-module
6ffff02
colesbury
colesbury approved these changes May 2, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 2, 2024
@colesbury
colesbury merged commit f8290df into python:main May 2, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 2, 2024
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@swtaarrs @SonicField
pythongh-116738: Make _codecs module thread-safe (python#117530)
7dba86e
The module itself is a thin wrapper around calls to functions in
`Python/codecs.c`, so that's where the meaningful changes happened:

- Move codecs-related state that lives on `PyInterpreterState` to a
  struct declared in `pycore_codecs.h`.

- In free-threaded builds, add a mutex to `codecs_state` to synchronize
  operations on `search_path`. Because `search_path_mutex` is used as a
  normal mutex and not a critical section, we must be extremely careful
  with operations called while holding it.

- The codec registry is explicitly initialized as part of
  `_PyUnicode_InitEncodings` to simplify thread-safety.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mpage mpage mpage left review comments
@colesbury colesbury colesbury approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@swtaarrs @mpage @colesbury
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.