Skip to content

gh-153291: Fix data race in readline.get_completer() and get_pre_input_hook()#153362

Open
harjothkhara wants to merge 2 commits into
python:mainfrom
harjothkhara:gh-153291-readline-get-completer-race
Open

gh-153291: Fix data race in readline.get_completer() and get_pre_input_hook()#153362
harjothkhara wants to merge 2 commits into
python:mainfrom
harjothkhara:gh-153291-readline-get-completer-race

Conversation

@harjothkhara

Copy link
Copy Markdown
Contributor

readline.get_completer() reads state->completer and returns a new reference without taking the module critical section, while set_completer() swaps the same field under the critical section with Py_XSETREF. On the free-threaded build a concurrent getter can be incrementing the refcount of a hook the setter is simultaneously dropping. get_pre_input_hook() has the same problem with state->pre_input_hook — those are the only two hook fields with both a Py_XSETREF-based setter and a Python-level getter, so this covers the whole Python-visible getter surface.

The fix adds @critical_section to both getters, the same way gh-126895 did for the rest of the module.

Verified with a free-threaded TSAN build (--disable-gil --with-thread-sanitizer): the reproducer from the issue reports the race on main (the readline_get_completer read vs the set_hook write) and runs clean with this change. The new tests in test_readline.py trigger the same TSAN reports without the fix and pass with it.

Deliberately not included here: get_begidx()/get_endidx() and the C callback paths (on_completion() etc.) read module state without the critical section too, but their writer/reader sides live in readline callbacks (flex_complete(), on_hook()), where taking the module critical section is a design question (it would serialize completion callbacks against all other readline calls) — that's a follow-up, not a one-line annotation.

I used AI assistance while working on this change; I've reviewed it and can explain it.

Comment thread Lib/test/test_readline.py Outdated
readline.set_pre_input_hook(my_hook)
self.assertIs(readline.get_pre_input_hook(), my_hook)

def test_get_completer(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this test necessary? It didn't fail on main for me.

@harjothkhara harjothkhara Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, that test just checks basic get/set behavior and passes on main, so I've dropped it. The free-threaded tests below are the ones that actually catch the bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also don't force push

…e_input_hook()

The setters store these hooks while holding the module critical section
(via set_hook's Py_XSETREF), but the getters read and Py_NewRef the same
fields without it. Annotate both getters with @critical_section, matching
the other readline functions (pythongh-126895).
@harjothkhara harjothkhara force-pushed the gh-153291-readline-get-completer-race branch from 80af1eb to 704749c Compare July 9, 2026 01:34
Comment thread Lib/test/test_readline.py Outdated
@threading_helper.requires_working_threading()
def test_free_threading_get_completer(self):
def completer(b):
b.wait()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of passing the barrier as an argument, you can just reference it directly:

Suggested change
b.wait()
barrier.wait()

You could then remove the arguments from the args parameter.
Same with the other test.

@brijkapadia

Copy link
Copy Markdown
Contributor

I think the tests need to get moved to test/test_free_threading so that it runs with TSan.

…hreading

The Argument Clinic getters are exercised there under the TSan CI job
(test_free_threading is in TSAN_TESTS; test_readline is not).
@harjothkhara

Copy link
Copy Markdown
Contributor Author

Moved them to Lib/test/test_free_threading/test_readline.py so the TSan job runs them. Verified they fail on main and pass with the fix.

@brijkapadia

Copy link
Copy Markdown
Contributor

This other test probably also needs to run with TSan:

def test_free_threading(self):
def completer_delims(b):
b.wait()
for _ in range(100):
readline.get_completer_delims()
readline.set_completer_delims(' \t\n`@#%^&*()=+[{]}\\|;:\'",<>?')
readline.set_completer_delims(' \t\n`@#%^&*()=+[{]}\\|;:\'",<>?')
readline.get_completer_delims()
count = 40
barrier = threading.Barrier(count)
threads = [threading.Thread(target=completer_delims, args=(barrier,)) for _ in range(count)]
with threading_helper.start_threads(threads):
pass

It might be a good idea to move it in this PR although that's probably better answered by someone else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants