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

Turn calls of __traverse__ into no-ops for unsendable pyclass if on the wrong thread #3689

Merged
merged 1 commit into from Dec 23, 2023

Conversation

adamreichold
Copy link
Member

Adds a "threadsafe" variant of PyCell::try_borrow which will fail instead of panicking if called on the wrong thread and use it in call_traverse to turn GC traversals of unsendable pyclasses into no-ops if on the wrong thread.

This can imply leaking the underlying resource if the originator thread has already exited so that the GC will never run there again, but it does avoid hard aborts as we cannot raise an exception from within call_traverse.

Closes #3688

Copy link

codspeed-hq bot commented Dec 22, 2023

CodSpeed Performance Report

Merging #3689 will not alter performance

Comparing unsendable-threadsafe-traverse (4dc6c16) with main (65f25d4)

🎉 Hooray! pytest-codspeed just leveled up to 2.2.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 78 untouched benchmarks

@adamreichold
Copy link
Member Author

@JRRudy1 Could you use a dependency specification like

pyo3 = { git = "https://github.com/PyO3/pyo3.git", branch = "unsendable-threadsafe-traverse" }

and verify that this would work for your use case? Thanks.

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, nice! Looks straightforward enough.

I wonder if there's a way to add a test for this. I suspect the most reasonable option, if we felt it worth it, would be to add a test which manually calls the traverse slot?

src/pycell.rs Show resolved Hide resolved
@adamreichold
Copy link
Member Author

I wonder if there's a way to add a test for this. I suspect the most reasonable option, if we felt it worth it, would be to add a test which manually calls the traverse slot?

Will give it a try.

I also noticed that this should be mentioned in the guide...

@adamreichold adamreichold force-pushed the unsendable-threadsafe-traverse branch 2 times, most recently from 2f37419 to 0435ce0 Compare December 23, 2023 08:50
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.

Looks great to me, thanks, great test 👍

@adamreichold adamreichold added this pull request to the merge queue Dec 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 23, 2023
…he wrong thread

Adds a "threadsafe" variant of `PyCell::try_borrow` which will fail instead of
panicking if called on the wrong thread and use it in `call_traverse` to turn GC
traversals of unsendable pyclasses into no-ops if on the wrong thread.

This can imply leaking the underlying resource if the originator thread has
already exited so that the GC will never run there again, but it does avoid hard
aborts as we cannot raise an exception from within `call_traverse`.
@adamreichold adamreichold added this pull request to the merge queue Dec 23, 2023
Merged via the queue into main with commit 8bef6e3 Dec 23, 2023
37 checks passed
@adamreichold adamreichold deleted the unsendable-threadsafe-traverse branch December 23, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants