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

Friendlier error messages for re-entrancy failures #66

Merged
merged 1 commit into from
May 5, 2022

Conversation

jamesbornholt
Copy link
Member

Currently, if code tries to re-acquire a lock it already holds, it
correctly panics but with an unhelpful error message about Shuttle's
internals. Instead, let's provide a nicer error message about why the
calling code is buggy.

This came up while trying out the super neat example from a
fasterthanlime blog post, which Shuttle has no trouble finding the
deadlock in, but wasn't giving a helpful error message for.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Currently, if code tries to re-acquire a lock it already holds, it
correctly panics but with an unhelpful error message about Shuttle's
internals. Instead, let's provide a nicer error message about why the
calling code is buggy.

This came up while trying out the super neat example from a
[fasterthanlime blog post], which Shuttle has no trouble finding the
deadlock in, but wasn't giving a helpful error message for.

[fasterthanlime blog post]: https://fasterthanli.me/articles/a-rust-match-made-in-hell
Copy link
Contributor

@bkragl bkragl left a comment

Choose a reason for hiding this comment

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

Neat example and usability improvement!

One thing I was thinking about is whether it makes sense to have a feature in Shuttle that does precise on-the-fly deadlock detection. Right now there might be a deadlock among some tasks early on in an execution, but we need to wait until the end of the execution to detect it. It might be helpful for debugging to stop the execution right when the deadlock happens.

The nice thing about the current check is that it is completely generic. What I'm proposing would require knowledge about the synchronization primitives (e.g., a combined resource allocation graph for both Mutex and RwLock).

Comment on lines +150 to +152
if *writer == me {
panic!("deadlock! task {:?} tried to acquire a RwLock it already holds", me);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should be an RwLock, like in the doc. Same below.

@jamesbornholt
Copy link
Member Author

One thing I was thinking about is whether it makes sense to have a feature in Shuttle that does precise on-the-fly deadlock detection. Right now there might be a deadlock among some tasks early on in an execution, but we need to wait until the end of the execution to detect it. It might be helpful for debugging to stop the execution right when the deadlock happens.

The nice thing about the current check is that it is completely generic. What I'm proposing would require knowledge about the synchronization primitives (e.g., a combined resource allocation graph for both Mutex and RwLock).

I bet we could get some of the same effect by just having the ability to print a stacktrace of all the deadlocked threads. I'll open a separate issue for that.

@jamesbornholt jamesbornholt merged commit 46dccb9 into awslabs:main May 5, 2022
@jamesbornholt jamesbornholt deleted the reentrant-errors branch May 5, 2022 17:01
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