gh-153056: Fix a data race compiling the string.Template pattern in free-threading builds

[tonghuaroot]

Fixes the free-threading data race in the linked issue. Template._compile_pattern() could read a pattern that another thread had just compiled and cached, then hand that already-compiled re.Pattern to re.compile(pattern, cls.flags | re.VERBOSE), which rejects a flags argument on a compiled pattern and raises a spurious ValueError. The fix returns the already-compiled pattern instead, which is idempotent and lock-free.

Testing

• Lib/test/test_free_threading/test_string_template_race.py races the lazy compile across many threads with threading_helper.run_concurrently (barrier-synchronized). It uses a throwaway subclass re-armed to the uncompiled sentinel each round, so the shared string.Template class is never mutated. Without the fix, the first round raises the ValueError on many of the racing threads; with it, 0.
• Lib/test/test_string/test_string.py::TestTemplate::test_precompiled_pattern is a deterministic, non-threaded regression that runs on every build: a subclass supplying an already-compiled pattern raised the ValueError at class definition before the fix and works after it.

Notes

• This also lets a subclass assign an already-compiled re.Pattern to pattern, where it previously raised at class definition. The commit message calls this out as a deliberate consequence rather than leaving it implicit.
• There is a separate, benign race on cls.flags (two threads may both store re.IGNORECASE); it does not affect correctness and is intentionally left out of scope to keep this change single-purpose.
• I have not applied a backport label; this likely wants a 3.14 backport, but I will leave that call to a maintainer.

• Issue: string.Template: data race compiling the pattern on the free-threaded build raises a spurious ValueError #153056

[picnixz]

I am not very fond of that but I don't see a better way of doing it except by allowing flags on a compiled pattern assuming those flags are the same. Are the flags retained by a pattern object or not? if they are not, I don't see another solution than the proposed one.

[warsaw]

It's actually a little more complicated because of what the docs actually say:

Alternatively, you can provide the entire regular expression pattern by overriding the class attribute pattern. If you do this, the value must be a regular expression object with four named capturing groups.

"regular expression object" implies a compiled regular expression, but that actually breaks in exactly the way you've identified in your "non-free-threading" comments and test. So the very thing that the docs tell you to do is unsupported, and I believe nothing actually tests that; all the tests set pattern to a regular expression pattern not a compiled object. Supporting a string pattern was the original supported behavior so at some point the docs got out of date.

So your fix is correct on that basis alone. Bonus that it fixes the FT race condition!

I'll take a closer look at the diff exactly, but since you're here, would you mind adding an update to the docs to describe that pattern can be a string pattern or a compiled regular expression object? With tests of the latter explicitly added, and your fix, I think it's worth backporting to 3.14 and 3.13.

[warsaw]

Thanks for finding and fixing this! As mentioned, I believe this is a legitimate bug regardless of the FT race, so it's good to fix and back port. Please do add a doc fix and ping me for a re-review when you're ready, as the bot will instruct.

[warsaw]

I think I'd add a comment here just above this line, something to the effect of:

# `pattern` can be one of three objects: a `_TemplatePattern` indicating that the descriptor has not yet
# run, a compiled regular expression object as stated in the docs, or a string representing a regular
# expression pattern.  If it's an already compiled regular expression object, we can just return that
# immediately.  For the former two cases, we have to compile the pattern and cache it back on the
# attribute.

[warsaw]

It'll be nice when we can get rid of all these and just use lazy imports 😄

[warsaw]

Also add that this fixes documented behavior that has always been broken? I'm not sure which to emphasize by mentioning first, but I'll leave that to you.

[bedevere-app]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[tonghuaroot]

Updated the docs for pattern to state it accepts a string or a compiled regex. The compiled-object case is covered by test_precompiled_pattern. Backport labels left to you.

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

[tonghuaroot]

Also addressed the inline comments: added the comment on the three states of pattern, and reworded the NEWS to lead with the documented-behavior fix.

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #33471246 | 📁 Comparing 764fa3f against main (35c6779)

  🔍 Preview build  

2 files changed

± library/string.html
± whatsnew/changelog.html