Skip to content
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 tests for cargo stress on free-threaded build #4469

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Aug 22, 2024

refs #4265

The free-threaded build has a lot of flaky tests at the moment and I'm trying various strategies to find them. Running cargo test in a bash while loop works well, but cargo stress also seems to find interesting bugs. This fixes issues that cargo stress hits quickly on my local setup.

The changes to test_datetime_imports can be hit in a regular gil-enabled python build, the test uses the filesystem assuming no other instances of the test function are running simultaneously. I updated it so it will create directories with unique names.

The changes for the decref pool tests are because in the free-threaded build other threads are free to process the decref pool at any time, it's no longer locked by the GIL. That's fine, it doesn't matter if the owning thread or another random thread processes the pool. It does mean that the assertions in the tests are no longer always true and depend on timing. I added comments where I had to think really hard about scenarios where the assertions don't hold.

There's also one assertion that is changed because the Py_DECREF calls on the pending decref pool can happen concurrently with calls to Py_REFCNT in other threads, making the result of Py_REFCNT racy. The mutex on the pending decrefs pool is implicitly released when pending_decrefs is dropped before calling the C API here:

pyo3/src/gil.rs

Lines 276 to 281 in 2f5b45e

let decrefs = mem::take(&mut *pending_decrefs);
drop(pending_decrefs);
for ptr in decrefs {
unsafe { ffi::Py_DECREF(ptr.as_ptr()) };
}

@ngoldbaum ngoldbaum added the CI-skip-changelog Skip checking changelog entry label Aug 22, 2024
@ngoldbaum ngoldbaum changed the title Fix tests for cargo stress Fix tests for cargo stress on free-threaded build Aug 22, 2024
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

tests/test_datetime_import.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@davidhewitt davidhewitt added this pull request to the merge queue Aug 23, 2024
Merged via the queue into PyO3:main with commit ec0853d Aug 23, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants