GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||
GeezerVison is showing github.com — gh-145446: Add critical section in functools module for `PyDict_Next` by brijkapadia · Pull Request #145487 · python/cpython Conversation
|
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @vstinner, @colesbury: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I don't think the remaining critical sections are needed either. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Okay, after thinking about it I see what you mean. I can close the PR if the critical sections are not needed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@colesbury After further reflection, I actually think that you need the critical section in the remaining cases. For example, without the critical section, this code results in a data race: from functools import partial from threading import Thread p = partial(lambda: None) def f(): for _ in range(100): repr(p) def g(): for i in range(100): p.keywords[f"{i}"] = i t1 = Thread(target=f) t2 = Thread(target=g) t1.start() t2.start() t1.join() t2.join() I attached the full TSan report here although it is quite long and I don't think it is that illuminating. However, the only remaining cases are for the Let me know if you think it is worth continuing this PR or if we should close it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for the example
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks @bkap123 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, @bkap123 and @encukou, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…`PyDict_Next` (pythonGH-145487) (cherry picked from commit 17eb035) Co-authored-by: bkap123 <[email protected]>
|
GH-145879 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.