Hang when reattaching after detach during shutdown#6085
Conversation
anuraaga
changed the title
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, in principle this seems ok.
General nit: a lot of the comments are very verbose, read like LLM-generated. If you did indeed use AI to help you produce this, please try to refrain from allowing the output to be too bloated.
| #[cfg(not(any(Py_3_14, target_arch = "wasm32")))] | ||
| struct HangThread; | ||
|
|
||
| #[cfg(not(any(Py_3_14, target_arch = "wasm32")))] | ||
| impl Drop for HangThread { | ||
| fn drop(&mut self) { | ||
| loop { | ||
| std::thread::park(); // Block forever. | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please move this to pyo3-ffi/src/impl_/mod.rs and deduplicate with the same logic in pystate.rs.
There was a problem hiding this comment.
Thanks! I wasn't sure what a good place for the shared logic would be will do that
| @pytest.mark.skipif( | ||
| sysconfig.get_config_var("Py_DEBUG"), | ||
| reason="causes a crash on debug builds", | ||
| ) | ||
| def test_detach_then_reattach_in_thread(): | ||
| # A Python daemon thread calls block_in_detach_until_finalizing(), which releases | ||
| # the GIL via py.detach() and polls Py_IsFinalizing() (safe without GIL — reads | ||
| # an atomic). When the interpreter begins finalizing, the thread exits the closure | ||
| # and SuspendAttach::drop() calls PyEval_RestoreThread() on a finalizing interpreter. | ||
| t = threading.Thread(target=misc.block_in_detach_until_finalizing, daemon=True) | ||
| t.start() | ||
| # No join — thread polls until finalization |
There was a problem hiding this comment.
Can this please have a similar mechanism to test_hammer_attaching_in_thread.py, perhaps both files can be merged together as test_finalization.py or similar?
| { | ||
| extern "C" { | ||
| fn _Py_IsFinalizing() -> std::ffi::c_int; | ||
| } |
There was a problem hiding this comment.
This will probably not work on PyPy or older x86 windows builds due to wrong symbol name. As per other comment I think this should be unified with test_hammer_attaching_in_thread.py
There was a problem hiding this comment.
I tried pretty hard to do that and it's in the first version of my issue comment. I couldn't avoid this different error
Fatal Python error: PyGILState_Release: thread state 0xefd390000910 must be current when releasing
Python runtime state: finalizing (tstate=0x000000000097a5d0)
I guess it could be about timing of being during GC or not. So I had to come up with a test case closer to the repro I got which was also polling until finalization. If you have any advice would be great
Sorry about that - the logic change was quite easy and I could do that manually but did many iterations with AI to get a test case that flipped with the change. While I tried to clean them up probably some things are more obvious than I thought, will trim more. |
| } | ||
|
|
||
| /// Wrapper to mark Receiver as Sync. | ||
| struct SyncReceiver<T>(std::sync::mpsc::Receiver<T>); |
There was a problem hiding this comment.
Incidentally I've always wondered if PyO3 could provide an easier way for this :-)
|
@davidhewitt I have gone ahead and merged the test with the existing pattern. Just as a reminder, the error message from this test is while the one I was debugging is But since the same fix applied to both, maybe this new test is close enough in behavior that it is effectively a regression test for both, though I'm not sure. Since I've confirmed it fixes my issue I am comfortable with it |
2 participants
Fixes #6084
By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.