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

Consider panicking on poisoned mutex #96

Closed
Plecra opened this issue Dec 19, 2023 · 3 comments
Closed

Consider panicking on poisoned mutex #96

Plecra opened this issue Dec 19, 2023 · 3 comments

Comments

@Plecra
Copy link
Contributor

Plecra commented Dec 19, 2023

https://github.com/khonsulabs/gooey/blob/63fd92eea6d612708db0da3be4b05a3de4535f10/src/value.rs#L774

I think this should panic? it probably means some thread has corrupted the Dynamic

@ecton
Copy link
Member

ecton commented Dec 19, 2023

I'm honestly torn on this.

A former part of me would have reached for parking_lot, which doesn't even support poisoning at all. Since std mutexes got optimized a while back, I stopped reaching for the extra dependency unless I needed one of their features. However, I still tended to ignore poisoning simply because I always was before, which is why you see me ignoring it.

I think at a minimum a feature flag to enable that panic is a good idea. But maybe you're right that we should just panic all the time. If we do, I think we should offer another lock call that returns a PoisonError rather than panicking. Maybe we can customize this with the type system somehow?

@Plecra
Copy link
Contributor Author

Plecra commented Dec 19, 2023

I think another function for returning the error is reasonable :) Of course, users can also catch_unwind if they want to deal with the exceptional situation.

This will only happen when someone attempted to use a Dynamic on this thread where something has already gone very wrong (one of the sibling threads is missing!). I expect panicking is the right behaviour for most users

@ecton
Copy link
Member

ecton commented Apr 5, 2024

As of 20ae2b7, Cushy now uses parking_lot. This was "needed" to solve some complex locking patterns that rely on my ability to use MutexGuard::unlocked() to temporarily unlock a mutex guard while a closure is executed. Because parking_lot doesn't support poison detection, this issue can't be handled at this time.

@ecton ecton closed this as completed Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

2 participants