-
Notifications
You must be signed in to change notification settings - Fork 806
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
fix flaky tests on free-threaded build #4576
Conversation
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! Think a couple of these might be overzealous, though the rest look fine. I guess the point is that the GC runs in a separate thread, so tp_clear
calls are done in the background?
tests/test_gc.rs
Outdated
#[cfg(not(Py_GIL_DISABLED))] | ||
assert!(drop_called1.load(Ordering::Relaxed)); | ||
#[cfg(not(Py_GIL_DISABLED))] | ||
assert!(drop_called2.load(Ordering::Relaxed)); |
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 test doesn't rely on a __clear__
function, so I think shouldn't be flaky with gc. Was this change out of an abundance of caution or because you did observe flakiness?
tests/test_gc.rs
Outdated
#[cfg(not(Py_GIL_DISABLED))] | ||
assert!(drop_called1.load(Ordering::Relaxed)); | ||
#[cfg(not(Py_GIL_DISABLED))] | ||
assert!(drop_called2.load(Ordering::Relaxed)); |
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.
Similarly no __clear__
/ gc integration here.
Thanks for pointing that out, that was me misunderstanding rather than being overzealous. |
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.
Understood, this LGTM now 👍
These asserts should all be skipped for the same reason as the assert that's skipped already.
Also moved the explanation to the docstring for the struct we use for this.