Fix issue with PyObject drop and allow_threads #913
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While writing tests for #908 I realised unfortunately there is a bad interaction between the optimization I wrote in #851 and
allow_threads
.With the
GIL_COUNT
method forgil_is_acquired
, it's not possible to know if we are inside anallow_threads
call without the GIL. The way #851 is currently written, in this situation we end up decreasing a reference count without the GIL held, which could cause all kind of bad stuff.I propose to fix this situation by introducing a linked-list of GIL states, which is updated by both
GILPool
andallow_threads
.This fix is still broken if someone wishes to use the
PyEval_SaveThread
APIs themselves manually. If you think we need to worry about the case of users using PyEval_SaveThread themselves, then I think the only sensible solution is to revert #851.I think the implementation here can still be cleaned up a lot. I wanted to open this PR to discuss the problem before I bothered to make the solution particularly tidy.
(I have also checked benchmarks and it does not noticeably affect performance.)
Let me know if you think I should finish this fix, or revert #851.