Skip to content

Fix __repr__ race (Free-Threaded)#1328

Open
Vizonex wants to merge 10 commits intoaio-libs:masterfrom
Vizonex:md-repr-ft
Open

Fix __repr__ race (Free-Threaded)#1328
Vizonex wants to merge 10 commits intoaio-libs:masterfrom
Vizonex:md-repr-ft

Conversation

@Vizonex
Copy link
Copy Markdown
Member

@Vizonex Vizonex commented Apr 28, 2026

What do these changes do?

Fixes another race condition seen in https://gist.github.com/devdanzin/fa00e03ef041c030660f083116a7e1b6#7-repr-race--md_repr Item 7.

Are there changes in behavior for the user?

Related issue number

#1321

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Vizonex Vizonex requested a review from asvetlov as a code owner April 28, 2026 20:34
@Vizonex Vizonex requested a review from webknjaz as a code owner April 28, 2026 20:35
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Apr 28, 2026
@Vizonex Vizonex changed the title fix __repr__ race (Free-Threaded) Fix __repr__ race (Free-Threaded) Apr 28, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 28, 2026

Merging this PR will not alter performance

✅ 245 untouched benchmarks


Comparing Vizonex:md-repr-ft (090f394) with master (4122516)

Open in CodSpeed

Copy link
Copy Markdown
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Structural comments.

Comment thread CHANGES/1328.bugfix.rst
Comment thread CHANGES/1328.bugfix.rst Outdated
import sysconfig
import textwrap

FREETHREADED = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be moved to the skipif mark in the pytest param.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do also since we seem to have a ton of bugs to patch. I was thinking about a new module called test_free_threaded.py when #1327 is merged to just have both of them in a new file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's see if @Dreamsorcerer and @bdraco have opinions on this, then.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good. :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Surely the tests should pass on regular versions of Python?

Vizonex and others added 3 commits April 28, 2026 15:54
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Comment thread CHANGES/1328.bugfix.rst


if __name__ == "__main__" and FREETHREADED:
child = textwrap.dedent("""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd also get rid of the double-subprocess indirectly here. If we want to invoke a bunch of Python code with different Python flags, we can put those flags into the test parametrization. Additionally, a chunk of Python in a string is not lintable, and the coverage is not getting measured. It should definitely live in a Python module as a first-class citizen.

cc @Dreamsorcerer

Vizonex and others added 4 commits April 29, 2026 20:45
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Comment thread CHANGES/1328.bugfix.rst Outdated
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
@Vizonex Vizonex added the free-threading FT-related discussion, issue or PR label May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR free-threading FT-related discussion, issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants