ping@ZeroIntensity ,@colesbury

Critical sections areper-interpreter, so this will continue to race in subinterpreters.

What I meant above is thatreadline just straight up doesn't import in a subinterpreter, it gives that error:module readline does not support loading in subinterpreters, so this is not an issue.

That said, I'm not sure whether it's unsafe to callanyreadline function concurrently,

You can call across threads and it won't crash because it is synchronized with GIL, you will just get wrong results. But if you called free-threaded it would crash, now it just behaves like not free-threaded and doesn't crash.

but otherwise we can go with a local static PyMutex to serialize single calls

Could do that, but is that safe if one call dies in an unexpected way and mutex is left locked?

• Oh, I thought we got everything in the stdlib over to multi-phase init :(. It might be worth fixing that and going with global locks for 3.14, but this fix makes sense for 3.13.
• Yes, but I'm trying to figure out the actual implications here. Can we call unrelatedreadline functions concurrently, or does all of readline need to be protected by a single lock?
• Critical sections have the same issue. We shouldn't worry about threads randomly dying and leaving things locked.

• Yes, but I'm trying to figure out the actual implications here. Can we call unrelatedreadline functions concurrently, or does all of readline need to be protected by a single lock?

The single lock onmodule is what I implemented here, so they are all mutually exclusive (since readline lib is specifically not thread-safe).

• Critical sections have the same issue. We shouldn't worry about threads randomly dying and leaving things locked.

I was under the impression that critical sections, as opposed to purePyMutex, are fairly deadlock proof? Don't they release after a period of time? And also:https://docs.python.org/3/c-api/init.html#python-critical-section-api -

"Critical sections avoid deadlocks by implicitly suspending active critical sections and releasing the locks during calls toPyEval_SaveThread(). WhenPyEval_RestoreThread() is called, the most recent critical section is resumed, and its locks reacquired. This means the critical section API provides weaker guarantees than traditional locks ..."

Critical sections are based off an object'sPyMutex, it just implicitly releases that mutex when the thread state is detached (for example, when waiting on another lock), which prevents deadlocks. If the thread crashes, the lock is still held. (These details don't matter much here, anyway.)

So what's the verdict then wrt this, changes?

Let's just confirm that we really do need a global lock here; I don't want to drastically inhibit scaling if we don't have to.

Let's just confirm that we really do need a global lock here; I don't want to drastically inhibit scaling if we don't have to.

Short: Yes it needs to be global because readline is not thread-safe. But its only applied on things that actually call into readline.

I'm fine with critical sections too, I'm just worried about basically enabling the GIL for all usage ofreadline.

Yes it needs to be global because readline is not thread-safe. But its only applied on things that actually call into readline.

I think you're misinterpreting what I mean a little. "Thread-safety" is a blanket term; I mean that we might not need locks synchronizing theentire module. For example, it makes sense to me thatwrite_history_file isn't thread-safe againstread_history_file, but I see no reason thatset_startup_hook couldn't run in another thread while those two are synchronized.

Adding a lock to the readline module is not like enabling the GIL. The GIL affects everything in the process, including things that don't touch the readline module. The readline lock only affects things that use readline. Using multiple threads to concurrently muck around with readline is not a real use case that we should optimize for. So far the only time I've seen anyone try to use readline from multiple threads at all is when fuzzing CPython.

...but I see no reason that set_startup_hook couldn't run in another thread while those two are synchronized...

In general, we should prefer the simplest approach to thread safety that is efficient. In this case, that means a single lock for the entire readline module.

read_history_file had critical section but I did misswrite_history_file andappend_history_file. Other points:

• @critical_section module ->@critical_section.
• Added atomics instead of critical sections to_history_length stuff.
• Removed tests but left one single minimized one because there really should be at least one to check for crash?
• readline_set_completion_display_matches_hook_impl has critical section because is settingrl_ global var which may be used inside locked readline functions. Anything that touches globalrl_ stuff is critical sectioned (except setup / constructor stuff).
• I didn't give stuffset_hook andset_startup_hook and the like critical sections because they just setting py vars, not readline stuff, maybe should have at least atomics, but does it matter?

One potential issue.call_readline() uses readline stuff but not locked here because is super low level and no clean way to access lock. Also not used here but exposed viaPyOS_ReadlineFunctionPointer which is used bymyreadline.c inParser. Could storemodule in a global to use for lock or look it up everytime insys.modules in this function, but is it worth it? Or can just count on caller to sync and that nothing else is usingreadline simultaneously?

• set_hook should not have it's own critical section, but the callers should have a critical section
• call_readline cannot (and should not) use a critical section. It's called without the GIL held in the default build.

🤖 New build scheduled with the buildbot fleet by@colesbury for commit0dca261 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131208%2Fmerge

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

Thanks Tom!

I'm not going to backport this to 3.13. I think it's unlikely that anyone is going to try to use the readline module from multiple threads concurrently outside of fuzzing.