Repository navigation
gh-124397: Add threading.iter_locked - #133908
eendebakpt wants to merge 13 commits into
Conversation
Sorry, something went wrong.
|
@serhiy-storchaka, what was the rationale behind putting this in the threading module? I'm not against it, but I'm more in favor of itertools because threading doesn't expose any other utilities like this. Maybe we should add a threading.util module? |
Sorry, something went wrong.
|
|
||
| The ``iter_locked`` makes non-atomic iterators atomic:: | ||
|
|
||
| class non_atomic_iterator: |
There was a problem hiding this comment.
I'm not sure non_atomic_iterator is a good example of something that exists in the wild, because it's not thread-safe on GIL-ful builds either. The thread can arbitrarily switch during execution of Python code, so this would need a lock anyway if concurrent iteration were a use case. For free-threading, __next__ itself should have its own locking to prevent races. How was this design reached?
Sorry, something went wrong.
There was a problem hiding this comment.
The iter_locked allows the non_atomic_iterator to be used in a threaded-setting, whether in the normal or FT build. Do you have suggestions for other tests?
Sorry, something went wrong.
|
Sorry, something went wrong.
|
Sorry, something went wrong.
|
|
||
| def __next__(self): | ||
| a = next(self.it) | ||
| b = next(self.it) |
There was a problem hiding this comment.
Add time.sleep(0) (or with small non-zero argument) or Event.wait() with a timeout to force or significantly increase the chance of concurrent access.
Sorry, something went wrong.
| iter_locked_object *lz = iter_locked_object_CAST(op); | ||
| PyObject *result = NULL; | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(op); // lock on op or lz->it? |
There was a problem hiding this comment.
Isn't it a no-op in the GIL build?
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch, it is indeed a no-op. The Py_BEGIN_CRITICAL_SECTION is defined using _PyCriticalSection_BeginMutex, but that is not available in the normal build. So we should either add it to the normal build, or use another lock implementation.
Sorry, something went wrong.
|
|
||
| Py_BEGIN_CRITICAL_SECTION(op); // lock on op or lz->it? | ||
| PyObject *it = lz->it; | ||
| result = PyIter_Next(it); |
There was a problem hiding this comment.
Let's use PyIter_NextItem, PyIter_Next has a weird return value.
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Why a simple Python implementation is not good enough?
What will happen if self-lock an iterator? Is there a way to forcefully unlock it ot prevent self-locking by raising an exception?
def g():
yield next(it)
it = iter_locked(g())
next(it)
Sorry, something went wrong.
Originally I wrote the iter_locked to be in itertools which only has a C module, hence the C implementation. But the only reason for a C implementation would be performance, which might not be very important here. I am happy to switch to a python version.
There was a deadlock, with no way to get out. I changed to a recursive lock and added 2 tests. The behaviour is now similar to the pairwise iterator. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
Continuation of #133272
Name was discussed in https://discuss.python.org/t/name-for-new-itertools-object-to-make-iterators-thread-safe/90394
📚 Documentation preview 📚: https://cpython-previews--133908.org.readthedocs.build/