gh-111965: Using critical sections to make io.StringIO thread safe.

[aisk]

Added the "critical sections" tags for most methods of the StringIO class, except the __new__ and __init__. The reference implementation from colesbury/nogil-3.12@6323ca60f9 dosen't do it also. I think it's because with the __new__ and __init__ process, other threads will not touch the newly created instance, so it does not need lock.

And for the getter closed and linebuffering, there are only a few read-only field access for conditioning (and will raise exception if failed), so it's not need to protect.

I don't know if I'm right for these two questions. And do we need some performance benchmark for the change? If so, I can provide some help later.

• Issue: Use critical sections to protect I/O objects (in --disable-gil builds) #111965

[colesbury]

Thanks @aisk.

I think we want critical sections for stringio_closed and stringio_closed for the accesses to closed because the object might be concurrent closed. You'll need to be careful because the CHECK_ macros may return and it's not safe to return from the middle of a critical section. It may be easiest to split those functions in two.

Similarly for stringio_newlines we want the critical section to cover at least the CHECK_INITIALIZED and CHECK_CLOSED.

Some basic performance measurements would not be a bad idea, but don't go overboard. They'll be more important after other changes to I/O objects.

[colesbury]

We don't need critical sections in __new__. The __init__ functions is a bit more debatable; in theory one can call __init__ on an already initialized object. For now I'd like to avoid locking/critical_sections in __init__, but we may want to revisit that later.

[aisk]

Updated, and made a small performance test on my local machine, and not seen any noticeable performance difference before and after the commits. I just realized that in the default build (no --disable-gil flag), the critical sections macros are just dummy macro, so there should not be performance issue.

[colesbury]

Thanks @aisk. What's the performance impact on --disable-gil builds using your benchmark?

I suggested a different structure for the three getter functions that I think will make them more consistent with the rest of the file (in particular, not having to inline CHECK_INITIALIZED/CHECK_CLOSED).

[aisk]

I found that it's really common for the getters and setters to add the critical section guard, and makes the work repeatability and error-prone.

Can we add some code generation process, like the argument clinic stuff, or just introduce a C macro to reduce the repeatability, like:

static PyObject * xxx_getter_impl(PyObject *self, void * context){// original codes. } Py_CRITICAL_SECTION_GETTER(xxx_getter); // Using this macro to generate the `xxx_getter` function and call the old `xxx_getter_impl` 

@colesbury

[colesbury]

@aisk, Yeah I think adding support for getters/setters to Argument Clinic is the way to do it.

[colesbury]

Thanks, this looks great!

@corona10, would you please review this as well?

[corona10]

ditto: #112193 (comment)

[corona10]

Suggested change

	state: object
	state: object
	/

[corona10]

@colesbury
Out of curiosity:
Should user explicitly care about thread-safe when implementing dunder methods of object for each case?
Or Will it be handled from interpreter side?

[corona10]

__setstate__ should be METH_O not METH_FASTCALL|METH_KEYWORDS
See: https://github.com/python/cpython/pull/112116/files#r1398121900

[aisk]

Just realized the difference, thanks a lot for point out

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[corona10]

Since we update AC tool, we don't have to add a header manually from now on.
#112251
Please rebase the PR and run make clinic one more time.

[aisk]

Updated