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

add a MutexExt trait to avoid deadlocks using Mutex::lock() #4934

Merged
merged 6 commits into from
Feb 28, 2025

Conversation

ngoldbaum
Copy link
Contributor

Fixes #4771.

I couldn't find any good uses for this inside PyO3 itself, but we know of at least two spots in rust-numpy this would be useful.

@ngoldbaum ngoldbaum added this to the 0.24 milestone Feb 24, 2025
@ngoldbaum ngoldbaum force-pushed the mutex-ext branch 2 times, most recently from aa42fca to c4de919 Compare February 24, 2025 23:10
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, looking good! One possible optimization opportunity for us to take, maybe?

&self,
_py: Python<'_>,
) -> std::sync::LockResult<std::sync::MutexGuard<'_, T>> {
// SAFETY: detach from the runtime right before a possibly blocking call
Copy link
Member

Choose a reason for hiding this comment

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

Instead of unconditionally detaching, is it worth calling self.try_lock() first, and returning eagerly in all cases except where it would block? (And then if it would block, continue to the code below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it can't hurt to avoid detaching and reattaching if it's possible. I'll go ahead and do that.

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Looks good to me now

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 too with one tiny code style suggestion, thanks 👍

Co-authored-by: David Hewitt <[email protected]>
@ngoldbaum ngoldbaum enabled auto-merge February 27, 2025 22:01
@ngoldbaum ngoldbaum added this pull request to the merge queue Feb 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 27, 2025
@ngoldbaum
Copy link
Contributor Author

Looks like a network error. One more try…

@ngoldbaum ngoldbaum added this pull request to the merge queue Feb 27, 2025
Merged via the queue into PyO3:main with commit 72f9e75 Feb 28, 2025
48 checks passed
@ngoldbaum ngoldbaum deleted the mutex-ext branch February 28, 2025 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MutexExt to avoid GIL deadlocks
3 participants