test_gc: refactor and silence flaky assertion on freethreaded build #4628
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidhewitt
commented
Oct 18, 2024
Comment on lines
+613
to
+637
| #[cfg(not(Py_GIL_DISABLED))] | ||
| { | ||
| // FIXME: seems like a bug that this is flaky on the free-threaded build | ||
| // https://github.com/PyO3/pyo3/issues/4627 | ||
| check.assert_drops_with_gc(ptr); | ||
| } | ||
|
|
||
| assert!(drop_called.load(Ordering::Relaxed)); | ||
| #[cfg(Py_GIL_DISABLED)] |
There was a problem hiding this comment.
cc @ngoldbaum
The check.assert_drops_with_gc helper is an addition higher up in the file. On freethreaded build it runs gc.collect() and then sleeps, in a loop. The interesting is no matter how long I let the loop run (e.g. 1 minute or more), this object never gets dropped. That's what makes me think it's likely a bug.
(It seems like __clear__ is never called, though I think I did observe __traverse__ being called.)
davidhewitt
force-pushed
the
flaky-gc-tests
branch
from
cf8c0ae to
16eceb4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a cleanup of the
test_gc.rstest suite after we have a flaky assertion in CI causing a lot of failed merges. (See #4627)Here I've just cleaned up the test suite to make more use of common code and silenced the flaky warning so it doesn't block us while we investigate later. I'll merge this now to unblock CI, if there are comments / suggestions post-merge I am happy to raise a follow-up PR.