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

Handle UNPARKING state in Receiver::drop() #60

Merged
merged 3 commits into from
Feb 22, 2025

Conversation

sandersaares
Copy link
Contributor

@sandersaares sandersaares commented Feb 21, 2025

I am observing that the unreachable!() branch is occasionally hit in Receiver::drop() under multithreaded load in async scenarios. This appears to be because the UNPARKING state is not handled in the match. Now it is.

I am not 100% confident in the correctness of the logic I added here - while the code in this crate is very well commented and proved easy to work with, I do not quite have the right state transitions mapped out in my head. Therefore, I just tried to follow what a similar block in a recv implementation above does. It eliminated this crash for me, at least, but perhaps I missed something. I am not even 100% convinced whether/when the UNPARKING state can be reached here (I cannot rule out a programming error in my code using oneshot wrongly).

@faern
Copy link
Owner

faern commented Feb 21, 2025

Interesting. Good find! I wonder if UNPARKING has been forgotten in more places, given that it's a state that was bolted on a bit later in the development process. Sadly the state itself did not even get a proper description of what it's for. I'll look into this and evaluate your proposed solution.

Do you have a minimal example that exhibits the crash panic that I can look at maybe?

@faern
Copy link
Owner

faern commented Feb 21, 2025

I wonder if we can write a loom test that catch this error. To prevent future regressions.

@faern
Copy link
Owner

faern commented Feb 21, 2025

I managed to write a test that proves the bug you encounter exists on main. When running this test towards your branch it passes. Feel free to cherry-pick this commit into your PR: b9bfaa6

You also have some CI failures to look into. Looks like you have to make sure hint is imported even when the async feature is not enabled now.

@faern faern merged commit 208cf17 into faern:main Feb 22, 2025
2 of 10 checks passed
@faern
Copy link
Owner

faern commented Feb 22, 2025

Thank you so much for the contribution! I went ahead and merged this together with my test. I'll make some minor adjustments, according to the feedback I left above. Hoping to have this out as a release fairly soon.

@faern
Copy link
Owner

faern commented Feb 22, 2025

Released as 0.1.11. Thank you for finding and fixing this bug. https://crates.io/crates/oneshot/0.1.11

@sandersaares sandersaares deleted the u/sasaares/handle-unparking branch February 22, 2025 09:43
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.

2 participants