X Tutup
Skip to content

gh-129983: fix data race in compile_template in sre.c#130015

Merged
kumaraditya303 merged 9 commits intopython:mainfrom
tom-pytel:fix-issue-129983
Feb 12, 2025
Merged

gh-129983: fix data race in compile_template in sre.c#130015
kumaraditya303 merged 9 commits intopython:mainfrom
tom-pytel:fix-issue-129983

Conversation

@tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Feb 11, 2025 •
edited
Loading

I am assuming the conversation was resolved on the side of the atomic?

I am not sure about the test added here for 3 reasons:

  1. Need to turn off all the other re tests because not everything there is free-thread safe yet.
  2. Need to make sure nothing uses re.sub() before this test so that re._compile_template doesn't get cached yet.
  3. The test itself is very non-deterministic by its nature, sometimes it will success when it should fail.

The other option would be to make a separate test file just for this case? Overkill I think. So opinion on this requested, maybe don't need test for this?

@bedevere-app bedevere-app bot mentioned this pull request Feb 11, 2025
Closed
colesbury
colesbury reviewed Feb 11, 2025
View reviewed changes
Modules/_sre/sre.c Outdated
{
/* delegate to Python code */
PyObject *func = module_state->compile_template;
PyObject *func = FT_ATOMIC_LOAD_PTR_RELAXED(module_state->compile_template);
Copy link
Contributor

@colesbury colesbury Feb 11, 2025

Choose a reason for hiding this comment

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

Let's use FT_ATOMIC_LOAD_PTR here.

FT_ATOMIC_LOAD_PTR_ACQUIRE would probably also be fine, but I think symmetry between the load and store is generally a good idea.

@colesbury
Copy link
Contributor

colesbury commented Feb 11, 2025

I think we can leave the test out for now. I briefly looked at this yesterday, and the other tests in test_re would catch this when run with --parallel-threads. The main problem, as you probably noticed, is that lots of the tests would fail because catch_warnings is not thread-safe (even with the GIL). I think that should be fixed relatively soon with #130010. When that's ready, we can enable test_re in TSAN_PARALLEL_TESTS. I'm not too worried about regressions in the meantime.

colesbury
colesbury approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

@colesbury
Copy link
Contributor

colesbury commented Feb 11, 2025

@kumaraditya303 - is this okay with you?

@tom-pytel
Copy link
Contributor Author

tom-pytel commented Feb 11, 2025

Thanks!

np :)

kumaraditya303
kumaraditya303 approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303 kumaraditya303 added the needs backport to 3.13 bugs and security fixes label Feb 12, 2025
@kumaraditya303 kumaraditya303 merged commit 3cf68cd into python:main Feb 12, 2025
50 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Feb 12, 2025

Thanks @tom-pytel for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Feb 12, 2025

Sorry, @tom-pytel and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3cf68cdd3e1809df4e426c61f6990de63747ec6f 3.13

kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Feb 12, 2025
@tom-pytel @kumaraditya303
pythongh-129983: fix data race in compile_template in sre.c (python#1…
6c54f89 
…30015)

(cherry picked from commit 3cf68cd)
kumaraditya303 added a commit that referenced this pull request Feb 12, 2025
@kumaraditya303 @tom-pytel
[3.13] gh-129983: fix data race in compile_template in sre.c (#130038)
4cb251d 
gh-129983: fix data race in compile_template in sre.c (#130015)

(cherry picked from commit 3cf68cd)

Co-authored-by: Tomasz Pytel <tompytel@gmail.com>
@tom-pytel
Copy link
Contributor Author

tom-pytel commented Feb 12, 2025

Anyone want backport or should I do it?

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Feb 12, 2025

Anyone want backport or should I do it?

I did that already #130038

@hugovk hugovk removed the needs backport to 3.13 bugs and security fixes label Feb 17, 2025
@tom-pytel tom-pytel deleted the fix-issue-129983 branch March 4, 2025 12:08
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

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Assignees

@kumaraditya303 kumaraditya303

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tom-pytel @colesbury @kumaraditya303 @hugovk
X Tutup