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 deadlock in update_counts #924

Merged
merged 4 commits into from
May 14, 2020
Merged

Fix deadlock in update_counts #924

merged 4 commits into from
May 14, 2020

Conversation

davidhewitt
Copy link
Member

This fixes the deadlock reported in the discussion in #916, which became possible after incomplete test coverage of the change made in #899.

I wrote the test first and so can confirm the deadlock did indeed exist before this bugfix.

@kngwyu
Copy link
Member

kngwyu commented May 14, 2020

Looks good, but I feel the combination of Lock and UnsafeCell redundant here. How about using parking_lot::Mutex or std::sync::Mutex?
Also, it would be good if @IvanKuzavkov could check this patch works.

@IvanKuzavkov
Copy link

@kngwyu @davidhewitt I've checked this fix, it works well - thanks.

@davidhewitt
Copy link
Member Author

I would be happy to use a mutex to reduce amount of unsafe code. For now we'll probably need to use parking_lot because std::sync::mutex cannot be easily constructed in a static without lazy_static. Also I think I read somewhere that parking_lot has better performance?

src/gil.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Changed to remove Option. I gotta head to work; if you want this branch merged today and want any changes to it, please feel free to change as you like 😄

src/gil.rs Outdated
// Get vec from one of ReferencePool's mutexes via lock, swap vec if needed, unlock.
($cell:expr) => {{
let mut locked = $cell.lock();
let mut out = None;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need Option here, too. Zero-sized Vec is internally a constant and fast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep agreed 👍 if you want to change this before I get a chance later tonight, please push to this branch 😄

Copy link
Member

Choose a reason for hiding this comment

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

Done 👍

@kngwyu kngwyu merged commit bc3fac9 into PyO3:master May 14, 2020
@kngwyu
Copy link
Member

kngwyu commented May 14, 2020

Thank you!

@kngwyu kngwyu mentioned this pull request May 14, 2020
@davidhewitt davidhewitt mentioned this pull request May 14, 2020
@davidhewitt davidhewitt deleted the fix-deadlock branch August 10, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants