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-116664: Ensure thread-safe dict access in _warnings by erlend-aasland · Pull Request #116768 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Mar 13, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Replace _PyDict_GetItemWithError() with PyDict_GetItemRef().

  • Issue: Make _warnings.c thread-safe in free-threaded build #116664

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland erlend-aasland added the skip news label Mar 13, 2024
@erlend-aasland
erlend-aasland requested a review from colesbury March 13, 2024 21:51
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 13, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Mar 13, 2024
Make _warnings.c thread-safe in free-threaded build #116664
Closed
erlend-aasland added 4 commits March 13, 2024 22:55
@erlend-aasland
In already_warned, replace _PyDict_GetItemWithError with PyDict_GetIt…
cda2f6f 
…emRef
@erlend-aasland
In setup_context, replace _PyDict_GetItemWithError with PyDict_GetIte…
d05c678 
…mRef
@erlend-aasland
In setup_context, replace _PyDict_GetItemWithError with PyDict_GetIte…
eeb0936 
…mRef
@erlend-aasland
In get_source_line, replace _PyDict_GetItemWithError with PyDict_GetI…
4d4371c 
…temRef
@erlend-aasland
erlend-aasland force-pushed the nogil/warnings-dict-getitem branch from cedf1cf to 4d4371c Compare March 13, 2024 21:55
colesbury
colesbury reviewed Mar 13, 2024
View reviewed changes
Comment thread Python/_warnings.c Outdated
goto handle_error;
}
else {
assert(Py_IsNone(*module) || PyUnicode_Check(*module));

@colesbury colesbury Mar 13, 2024

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.

I think we need to handle the case where *module (i.e., __name__) is not None and not a unicode object because Python code can set __name__ arbitrarily.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@erlend-aasland erlend-aasland Mar 13, 2024

Copy link
Copy Markdown
Contributor Author

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.

4f3cd02 should fix that case:

If None or a unicode object, return successfully; else decref and fall through to the no-key case.

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

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.

Note, there is no test case for this in the test suite.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland
Address review: handle case where __name__ is not None and not a unic…
4f3cd02 
…ode object
colesbury
colesbury approved these changes Mar 14, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 14, 2024
@erlend-aasland
Merge branch 'main' into nogil/warnings-dict-getitem
062ec1a
@erlend-aasland
erlend-aasland enabled auto-merge (squash) March 18, 2024 09:10
@erlend-aasland

erlend-aasland commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the review!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland merged commit 762f489 into python:main Mar 18, 2024
@erlend-aasland
erlend-aasland deleted the nogil/warnings-dict-getitem branch March 18, 2024 09:37
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 18, 2024
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@erlend-aasland @vstinner
pythongh-116664: Ensure thread-safe dict access in _warnings (python#…
083ad3d 
…116768)

Replace _PyDict_GetItemWithError() with PyDict_GetItemRef().
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@erlend-aasland @adorilson
pythongh-116664: Ensure thread-safe dict access in _warnings (python#…
9bc76f6 
…116768)

Replace _PyDict_GetItemWithError() with PyDict_GetItemRef().
@erlend-aasland erlend-aasland linked an issue Mar 28, 2024 that may be closed by this pull request
Make _warnings.c thread-safe in free-threaded build #116664
Closed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@erlend-aasland @diegorusso
pythongh-116664: Ensure thread-safe dict access in _warnings (python#…
93d77a7 
…116768)

Replace _PyDict_GetItemWithError() with PyDict_GetItemRef().
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

Uh oh!

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

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make _warnings.c thread-safe in free-threaded build

Uh oh!

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

2 participants

@erlend-aasland @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.