-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-127085: Fix memoryview->exports
race condition.
#127412
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
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
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.
Thanks for contributing! I'm always happy to see new contributors 😄. Please address my comments and let me know if you have any questions.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Please add a news entry so the bot stops spamming. |
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.
Just some formatting nitpicks. I'll do a more thorough audit of the thread safety later today.
Hi @ZeroIntensity , Thanks very much for your patience and warm help! 😊 Wish a nice day! Best Regards, |
@LindaSummer Please do not update the branch with the latest main unless you have conflicts to resolve. This wastes resources and notifies everyone subscribed to the PR. |
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.
OK, on the basis that there will be a follow-up PR, this LGTM. Thanks, @LindaSummer!
Hi @ZeroIntensity , @picnixz , Thanks again for your guidance and patience! ❤ Best Regards, |
Hi @ZeroIntensity , Sorry to bother you again. 😊 This PR has been pending for three days in awaiting core review. Should I mention someone in this thread for next steps? 😊 Best Regards, |
No worries. Typically the |
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.
Sorry, I didn't notice the blurb before. That needs to get changed.
Misc/NEWS.d/next/Core_and_Builtins/2024-11-30-23-35-45.gh-issue-127085.KLKylb.rst
Outdated
Show resolved
Hide resolved
I'll be reviewing this by next week |
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.
LGTM, this doesn't fix all of the thread safety issues but is a step in right direction so I'm merging it
Thanks @LindaSummer for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, @LindaSummer and @kumaraditya303, I could not cleanly backport this to
|
I'll take of backporting this if needed as part of #127716 |
|
|
|
iOS and Android don't support subprocesses, and therefore don't support the |
Hi @mhsmith , Thanks for your suggestion! 😊 I will try to fix the test case issue. Best Regards, |
@kumaraditya303 Triage: Please could you make the backport if needed? Otherwise let's remove the label. Thanks! |
Issue
Close #127085 .
Proposed Changes
memoryview->exports
protected by critical section.Cause of the problem
With TSAN, I find that the root cause should be racing between
memory_getbuf
andmemory_releasebuf
.After adding the critical section, this racing has been fixed.
Here are the TSAN outputs.
ShareableList.count
in threads aborts:Assertion 'self->exports == 0' failed
#127085