GeezerVison

!buildbot nogil refleak

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit b6f2011 🤖

Results will be shown at:

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

The command will test the builders whose names match following regular expression: nogil refleak

The builders matched are:

• s390x Fedora Rawhide NoGIL refleaks PR
• AMD64 CentOS9 NoGIL Refleaks PR
• aarch64 Fedora Rawhide NoGIL refleaks PR
• PPC64LE Fedora Rawhide NoGIL refleaks PR
• AMD64 Fedora Rawhide NoGIL refleaks PR

The problem is that we were testing both _pyio.StringIO and _io.StringIO. The former, in _pyio, isn't really thread-safe (but it doesn't crash, and that's what we care about)

Sorry for challenging your changes recently, but why don’t we ensure consistent behavior between the pure Python and C versions?
Can we first focus on preventing the segmentation fault, rather than introducing behavioral differences between the two implementations? And just document it that we do not gurentee the thread safe?

Please hold on: "IMO this change was merged too quickly and I suggest to revert it." #135412 (comment)

Sorry for challenging your changes recently, but why don’t we ensure consistent behavior between the pure Python and C versions?

The pure Python will not crash the interpreter while the C implementation can crash and the easiest way to fix the latter is to add critical section. We are trying to fix thread safety in regard to avoid being able to crash the interpreter.

Another somewhat related is #130095 where abc's pure Python implementation is not thread safe but it cannot crash the interp so we aren't actively fixing that, rather skipping the pure python tests of it.

As for performance, critical sections are optimized for case where single thread acquires it so it is minimal, trying to write to same StringIO object would perform worse but that's not something users should do or we should try to optimize for.

Sorry for challenging your changes recently

No worries, I don't take it personally :)

Can we first focus on preventing the segmentation fault, rather than introducing behavioral differences between the two implementations? And just document it that we do not gurentee the thread safe?

That's effectively what I was trying to do. #135412 was trying to solely fix the segfault. Unfortunately, the only easy way to do that is by slapping a critical section onto __next__, which incidentally makes it work pretty sane across threads. I'm not a huge fan of it either, but I can't think of a different way to do this.

I agree with Kumar that this probably won't be noticeable for single-threaded code. I pushed back on locking TextIOWrapper.__next__ a few weeks ago because of performance, but the reality is that critical sections are quite fast when they're uncontended.

The pure Python will not crash the interpreter while the C implementation can crash and the easiest way to fix the latter is to add critical section. We are trying to fix thread safety in regard to avoid being able to crash the interpreter.

I know pure Python code doesn’t crash the interpreter, but that’s not the point. The problem is that it behaves differently from the C implementation: the Python version is not thread-safe, while the C version is. We should aim for consistent behavior between them and consider what we’ve done so far toward that goal. That’s why I suggested we revisit our approach to make the behavior consistent between the pure Python and C implementations.

Closing now that the original change has been reverted.