Skip to content

gh-123828: fix data raceing for _waiting_release method in _interpchannelsmodule.c #124107

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 1, 2025
Merged

gh-123828: fix data raceing for _waiting_release method in _interpchannelsmodule.c #124107

merged 9 commits into from
Oct 1, 2025

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Sep 15, 2024 •
edited by bedevere-app bot
Loading

@Zheaoli
pythongh-123828: fix data raceing for _waiting_release method in _int…
ffee4ed 
…erpchannelsmodule.c

Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli Zheaoli force-pushed the manjusaka/fix-123828 branch from d8a89c1 to ffee4ed Compare September 15, 2024 17:36
@Zheaoli Zheaoli changed the title gh-13828: fix data raceing for _waiting_release method in _interpchan… gh-123828: fix data raceing for _waiting_release method in _interpchannelsmodule.c Sep 15, 2024
@bedevere-app bedevere-app bot mentioned this pull request Sep 15, 2024
Closed
picnixz
picnixz reviewed Sep 15, 2024
View reviewed changes
@ZeroIntensity
Copy link
Member

@picnixz out of the scope of this PR, but would it be feasible to migrate things using PyThread locks over to PyMutex in the future?

@picnixz
Copy link
Member

picnixz commented Sep 17, 2024

Maybe. Haven't had the time to think about it and I don't know if it's possible for every case. Could you open another issue for that? also how would performances / maintenance cost be impacted?

@ZeroIntensity
Copy link
Member

Maybe. Haven't had the time to think about it and I don't know if it's possible for every case. Could you open another issue for that? also how would performances / maintenance cost be impacted?

It's probably worth discussing on discourse before opening an issue. AFAIK, PyMutex is supposed to be much more efficient than pthread mutexes.

@Zheaoli
Merge branch 'main' into manjusaka/fix-123828
398e021
@Zheaoli
Add news
5d929f6 
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli
Copy link
Contributor Author

Zheaoli commented Sep 27, 2025

@picnixz May I ask if this pr is worth to merge?

@picnixz
Copy link
Member

picnixz commented Sep 27, 2025

I am not well-versed enough in this part of the code to be sure that it's correct, so I'll let Eric decide (or @ZeroIntensity)

@ZeroIntensity
Copy link
Member

The core change looks correct, I noticed this with Eric at the sprint. Please add a test though.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Sep 27, 2025

The core change looks correct, I noticed this with Eric at the sprint. Please add a test though.

Sorry about this. I have noticed that seems We don't have tests for the data race before(correct me if I'm wrong). You mean that we enable some test in CI?

@ZeroIntensity
Copy link
Member

Just add something that stresses this concurrently, we'll get around to enabling TSan on test_interpreters later.

ZeroIntensity
ZeroIntensity reviewed Sep 27, 2025
View reviewed changes
Misc/NEWS.d/next/Core_and_Builtins/2025-09-27-16-16-15.gh-issue-123828.UDxaxV.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this news entry. _interpchannels isn't exposed to any user-facing API.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Sep 28, 2025

Just add something that stresses this concurrently, we'll get around to enabling TSan on test_interpreters later.

This issue is triggered by TSAN test. So I think if we don't need add more test if we are planning to enable TSAN

@picnixz
Copy link
Member

picnixz commented Sep 28, 2025

I think @ZeroIntensity wanted add a test that specifically triggers it, not one that indirectly triggers it.

@Zheaoli
recovery test
ddedbe6 
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli
remove news
18186c9 
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli Zheaoli marked this pull request as draft September 28, 2025 12:38
@Zheaoli
lint code
95d326c 
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli
lint code
7d55aa2 
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli Zheaoli marked this pull request as ready for review September 28, 2025 13:14
@Zheaoli
Copy link
Contributor Author

Zheaoli commented Sep 28, 2025

@ZeroIntensity @picnixz Actually I think we don't need new tests anymore. In #129824, we found that the current test will triger the TSAN report. And this PR can fix the test and I reenable the test now

ZeroIntensity
ZeroIntensity approved these changes Sep 28, 2025
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Ok, thanks, this looks good.

@ericsnowcurrently This is the race I was telling you about at the sprints. As far as I can tell, this is the only thing blocking test_interpreters from running on the free-threaded build.

@ZeroIntensity ZeroIntensity enabled auto-merge (squash) October 1, 2025 11:03
@ZeroIntensity ZeroIntensity merged commit f39dea3 into python:main Oct 1, 2025
43 checks passed
@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 1, 2025

The test has been failed https://buildbot.python.org/#/builders/75/builds/3415

I'm trying to figure it out

@Zheaoli Zheaoli deleted the manjusaka/fix-123828 branch October 1, 2025 13:06
@ZeroIntensity
Copy link
Member

That might be the leak from #139073.

@ericsnowcurrently
Copy link
Member

Any reason not to backport this to 3.14?

@ZeroIntensity
Copy link
Member

I didn't think it was that big of a deal because _interpchannels is only used in tests at the moment, but I don't mind backporting it. We'll have to wait until 3.14.1 because 3.14 is currently locked down for the RC, though.

@ZeroIntensity ZeroIntensity added the needs backport to 3.14 bugs and security fixes label Oct 2, 2025
@miss-islington-app
Copy link

Thanks @Zheaoli for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2025
@Zheaoli @miss-islington
pythongh-123828: Fix data race in _interpchannels._waiting_release (p…
1f12623 
…ythonGH-124107)

(cherry picked from commit f39dea3)

Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
@bedevere-app
Copy link

bedevere-app bot commented Oct 2, 2025

GH-139517 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Oct 2, 2025
ZeroIntensity pushed a commit that referenced this pull request Oct 7, 2025
@miss-islington @Zheaoli
[3.14] gh-123828: Fix data race in _interpchannels._waiting_release (…
a69bdab 
…GH-124107) (GH-139517)

gh-123828: Fix data race in `_interpchannels._waiting_release` (GH-124107)
(cherry picked from commit f39dea3)

Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TSAN race in _waiting_release

4 participants

@Zheaoli @ZeroIntensity @picnixz @ericsnowcurrently