-
Notifications
You must be signed in to change notification settings - Fork 834
hang instead of pthread_exit during interpreter shutdown #4874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #4874 will not alter performanceComparing Summary
|
80f138a
to
704ef0b
Compare
Yay it passes tests. |
pytests/tests/test_othermod.py
Outdated
@@ -3,7 +3,7 @@ | |||
|
|||
from pyo3_pytests import othermod | |||
|
|||
INTEGER32_ST = st.integers(min_value=(-(2**31)), max_value=(2**31 - 1)) | |||
INTEGER32_ST = st.integers(min_value=(-(2**30)), max_value=(2**30 - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change here is to avoid the test failing due to a high rate of assume
failure (filter_too_much
).
split the test_double fix to #4879 |
any update? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but I would like at least one other maintainers opinion (maybe @davidhewitt?) before proceeding here.
Please forgive the delay, I will seek to review this on Friday, and ping me repeatedly from then if I do not achieve that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think let's move forward with this. The cfg guards hack around the various edge cases and, as I read it, the interaction with pthread_exit and rust destructors was (and still is) UB, so this just creates a practical solution for now. Ultimately 3.14 will make this problem go away.
So the scary failure here is the CodSpeed regression? |
The failed stage is I assume it's probably fine and we should just skip the new test on debug builds? After all, we're trying to test a bad edge case. |
Agreed RE flaky benchmark, I will set that one to be ignored. |
This looks bad. Not sure it's our fault tho. Trying to investigate - if it's PyO3's fault, we should fix it. I want to know whether it's a CPython or a PyO3 problem. |
I didn't manage to debug it easily, will try more after Rust Nation. |
Looks like the failure does not reproduce on the builder either. I say merge this unless someone can reproduce the failure. |
ed: I can reproduce the assertion locally |
Backtrace:
|
It looks like
|
My feeling is that my change doesn't make the problem worse, and we should skip this test on debug builds. What's the way to do that? |
Understood, thanks for checking that. I've pushed something and will click auto-merge now 👍 |
As you can see in the python/cpython#131012 the assertion failure is currently expected on the CPython side. |
It looks like Any idea what the appropriate way to handle this is? Should we be using |
Replied in #4811 (review) |
This mimics the Python 3.14 behavior and avoids crashes in Rust 1.84.
See python/cpython#87135 and rust-lang/rust#135929 and the related pyo3-log issue vorner/pyo3-log#30
By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.