gh-133885: Use locks instead of critical sections for _zstd #134289
Conversation
Also remove critical sections around code only run in initializer.
|
@ZeroIntensity - |
|
Oh, that's news to me. When did that change? |
|
@ZeroIntensity - that's always been the case, as far as I know. A lot of things would break if |
Ah, that's what I was thinking of. We do need to document this contract somewhere. I'm imagining a case where an embedder sets their own allocator that calls Python code, because they have an attached thread state. The documentation doesn't say you can't do that, as far as I can tell. |
Using PyDict_SetDefaultRef allows us to not have contention on the capsules dictionary shared by ZstdDict instances.
There was a problem hiding this comment.
Ok, I understand this a little better now, sorry!
I'm a bit iffy about the lock-ordering here. I think the lock is only held under non-reentrant calls? PyErr_SetString and friends seem to be safe, but it looks like they might be able to re-enter if someone did weird things to the exception state. I wouldn't worry about it, really, but it's worth bringing up.
This reverts commit 005d1a0.
|
Are we sure about the implications of implicit locking in de/compressor contexts? If two threads concurrently write into a compressor context with implicit locking they might succeed in corrupting their data. Say:
Or pick whichever other interleaving of the two threads. This is just one example, but I think there's reasonable expectation that users won't want to share de/compressor contexts anyways. I think raising an exception when detecting concurrent use is a more desirable outcome. |
I think I would say don't write different data to the same stream with threading. I can append things to a shared list that makes it invalid for what I want but that is a programmer error. I don't particularly see a reason to share a compression context between threads, but I do think we might see people move a compression context into a thread to do all the compression in another thread, and we should try to support that. |
This also reverts the setdefault usage which we don't need anymore and refactors the _zstd_load_(c,d)_dict functions to not use goto/properly lock around dictionary usage.
|
Oh yes, I agree. Let me explain a bit better what my intention was with python-zstandard. Instead of using a lock-based approach, I used a flag to signify that a de/compressor context was in use by some thread. The differences with using locks are:
And, in both approaches:
IIUC, you alluded to an ownership transfer model. Note that this approach is not in contrast with that model. If a de/compressor object is created by one thread and then transferred to a different thread for use, no exception is raised. The only problematic usage that the exception would signal is when a thread calls into a context method while another thread was already using the context. In fact, this is not even in contrast with other thread synchronization mechanisms that don't use ownership transfer (e.g. barriers): if the program makes sure that there is no concurrent access, then the exception is never raised. Of course, the choice is up to you. I just wanted to make sure we considered both approaches. |
|
Hmm, I don't think we should be inventing object ownership models here. That makes sense for a third-party, but that sort of thing should get its own proposal for the stdlib as a whole. Locking should be sufficient, right? If torn writes at the method-level are an issue, we should just document that users need to have an additional layer of synchronization when using compressors concurrently. |
- Assert locks are held in `_get_(C/D)Dict` - Update self->d_dict outside `Py_BEGIN_ALLOW_THREADS` - removed critical section AC I missed
|
!buildbot nogil refleak |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit d142aa8 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134289%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
|
!buildbot nogil refleak |
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 69a755b 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134289%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
Hm, that CentOS 9 buildbot doesn't have zstd installed but it is failing due to unrelated issues. |
|
(FWIW, I ran the refleak hunter locally on a nogil build and it passed) |
|
Opened #134557 for the free threaded buildbot refleak |
|
Thanks @emmatyping for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…thongh-134289) Move from using critical sections to locks for the (de)compression methods. Since the methods allow other threads to run, we should use a lock rather than a critical section. (cherry picked from commit 8dbc119) Co-authored-by: Emma Smith <emma@emmatyping.dev>
|
GH-134560 is a backport of this pull request to the 3.14 branch. |
|
Thanks all for the reviews! Thinking through free threading reentrancy at the C level is something I'm still getting a feel for. Chatting about this fix has definitely helped with my understanding :) |
Move from using critical sections to locks for the (de)compression methods. Since the methods allow other threads to run, we should use a lock rather than a critical section.
I'm not totally sure about the removal of the critical sections in the
_get_(C|D)Dictfunction, but it should be safe to yield to other threads in an initializer as the values cannot be modified (ZstdDicts are immutable).test_zstdfailed on ubuntu with free-threading #133885