Skip to content

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