Currently on vacation but reviewed some high level pieces as a first pass

Thanks very much, but please don't feel any obligation to look at this while you are on holiday! There is no particular hurry, it can all wait 😄

As I said in the issue, concurrent iteration generally isn't worth fixing on our end.

Mentioned there also, but FTR this commit is just trying to fix the crash, not make the iterator thread-safe in any wider sense. I completely agree that is not a sensible goal. You will note there is no concurrent access of iterators in the reproduction script or test code!

It doesn't look like we do this for any of the other _io types.

If any of the other _io types are crashing with multi-threaded use...well, I'd be happy to try and fix them next 😉

You will note there is no concurrent access of iterators in the reproduction script or test code!

It concurrently iterates over f.

If any of the other _io types are crashing with multi-threaded use...well, I'd be happy to try and fix them next 😉

I'm pretty sure nearly all extension types will crash with concurrent iteration right now, because very few of them have locking. I think it's intentional for performance reasons.

That said, I think we can make this thread-safe without a critical section. AFAICT, the only thread-unsafe thing here is the telling field. Let's switch that to an atomic. I'm not worried about torn or inconsistent reads because the data will be pretty screwed up for concurrent readers anyway.

I think the direct readline call needs a critical section around it? (_io_TextIOWrapper_readline_impl which exposes it more directly has one)

    if (Py_IS_TYPE(self, self->state->PyTextIOWrapper_Type)) {
        /* Skip method call overhead for speed */
        line = _textiowrapper_readline(self, -1);
    }

Thoughts on doing: _Py_AssertHoldsTstate() to ensure/validate that it's locked in _textiowrapper_readline, removing the "skip method call overhead" shortcut (so it gets the critical section without more copy/paste), and the telling atomic?

(Probably would/should need to do a single-threaded perf test to validate removing the shortcut path isn't highly detrimental in text line iteration...)

Thoughts on doing: _Py_AssertHoldsTstate() to ensure/validate that it's locked in _textiowrapper_readline, removing the "skip method call overhead" shortcut (so it gets the critical section without more copy/paste), and the telling atomic?

_Py_AssertHoldsTstate will always pass here, that's for whether or not the thread has an attached thread state. I think you're right that we need a critical section around that call. Let's just call _io_TextIOWrapper_readline_impl to let AC do the work.

That said, I think we can make this thread-safe without a critical section. AFAICT, the only thread-unsafe thing here is the telling field. Let's switch that to an atomic.

I'm afraid that won't work: atomicity of that field by itself is not sufficient. The other methods that check or change it are written with the assumption it cannot change value while they are running. A critical section is specifically required here: that is what the class is using to synchronise access to its internal data.

Yeah, I'm suggesting you switch to atomics there too. This is only for telling, right?

Yeah, I'm suggesting you switch to atomics there too. This is only for telling, right?

I'm a bit worried we may be talking at cross-purposes 😅 Let me recap a bit, to make sure we are all talking about the same things...

As I understand it, the proposal is to call the argument clinic-generated readline function from textiowrapper_iternext, thereby ensuring it takes the critical section, and to make access to telling atomic (required since it is set inside textiowrapper_iternext and hence would not be protected by the critical section).

If I've misunderstood anything there, apologies, and please correct me.

However, to explain why I don't think the approach above will work: we don't need to just make one read/write safe, we need to protect a whole sequence of operations. I.e. we need a lock, which in this case should be the critical section. To illustrate, consider textiowrapper_read_chunk. It checks if (telling) twice. The first time it gets some state from the decoder and unpacks it. The second time it uses that data to update its own internal state following the read. If this function can race with textiowrapper_iternext then it doesn't matter if reading and writing to telling is atomic: if it changes halfway through bad things will happen. Similar issues arise elsewhere telling is accessed.

If this function can race with textiowrapper_iternext then it doesn't matter if reading and writing to telling is atomic: if it changes halfway through bad things will happen.

I don't think we should care about this case. As I said, concurrently reading from an iterator isn't something that's generally useful. You need your own synchronization anyway, so all we need to do is make sure that it doesn't crash. Without the caller holding a lock, threads will get torn reads regardless of how we make it thread-safe.

What's the actual practicality of concurrent reading from a file while randomly writing to it? My main concern here is that we're slowing down single-threaded code for a use-case that doesn't exist.

I don't think we should care about this case. As I said, concurrently reading from an iterator isn't something that's generally useful. You need your own synchronization anyway, so all we need to do is make sure that it doesn't crash. Without the caller holding a lock, threads will get torn reads regardless of how we make it thread-safe.

It will crash. Sorry for not being clearer. The possibilities under "bad things" in that function certainly include leaking object references; I am not sure if anything worse can happen there. Elsewhere assertion failures, use-after-free, and general arbitrary memory corruption are possible. For example, the Py_CLEAR(self->snapshot) at the bottom of textiowrapper_iternext_locked will certainly crash if it hits the right window racing with tell.

What's the actual practicality of concurrent reading from a file while randomly writing to it? My main concern here is that we're slowing down single-threaded code for a use-case that doesn't exist.

I'm not suggesting there is any legit reason to do this. It sounds like a really bad idea to me. As I said before, I don't really care if the result is gibberish. However, it should not crash the interpreter!

As for performance: my proposed fix takes a critical section on entering textiowrapper_iternext. This will have no impact on single-threaded performance on GIL builds and de minimis on free-threaded builds. Note that the alternative of calling _io_TextIOWrapper_readline_impl also takes the same critical section and needs to marshal arguments into PyObjects and back out again.

Probably would/should need to do a single-threaded perf test to validate removing the shortcut path isn't highly detrimental in text line iteration...

A good suggestion. A quick and very dirty benchmark shows no significant performance difference between this branch and where it diverged:

import io
import timeit

LINES = 1_000_000
bytes = b"\n" * LINES

def test():
    buffer = io.BytesIO(b"\n" * LINES)
    wrapper = io.TextIOWrapper(buffer)
    for _ in wrapper:
        pass

if __name__ == '__main__':
    timer = timeit.Timer(test)
    elapsed = timer.timeit(number=100)
    print(f"{elapsed:.2f}")

On my system, with an optimised free threading build, that is ~3.7ns per call, with or without the fix.

For example, the Py_CLEAR(self->snapshot) at the bottom of textiowrapper_iternext_locked will certainly crash if it hits the right window racing with tell.

That should get a critical section around it, but it's not the common path.

de minimis on free-threaded builds.

Yeah, the acquisition of a critical section itself is efficient. But critical sections come with a lot more bells and whistles than you'd think. In particular, every time the thread state is detached, the thread has to drop the lock, and then re-acquire that lock upon reattaching. The thread state is detached more often than you'd think; it happens between bytecode instructions, and also happens every time a mutex or other critical section is acquired.

needs to marshal arguments into PyObjects and back out again.

Hm? There's no marshalling there, AFAICT. Using the AC version is just a simpler way to automatically wrap the critical section. I don't have much of a preference whether we're explicit or implicit there.

Basically, my suggestion is to keep the locking in places where we only really need it to prevent crashes. iternext has a lot of paths that don't require locking, and it's something that will only be called concurrently when fuzzing Python. This fix works for simplicity, but it also sets a bad example, because iternext generally isn't something that should have to worry about concurrency.

Thanks @duaneg for the bug report and fix and everyone else for the code reviews!

Thanks @duaneg for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

Thanks @duaneg for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

Sorry, @duaneg and @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 44fb7c361cb24dcf9989a7a1cfee4f6aad5c81aa 3.13

Thanks @duaneg for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

GH-135039 is a backport of this pull request to the 3.14 branch.

GH-135040 is a backport of this pull request to the 3.13 branch.