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

servers: fix possible deadlock #3337

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Conversation

BurtonQin
Copy link
Contributor

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @BurtonQin ! This look good to me 👍.

@hashmap
Copy link
Contributor

hashmap commented Jun 3, 2020

While this change is fine I'm not sure about a deadlock here. Perhaps I'm missing smth, could you clarify it for me?

The first Read lock is not released before calling the second lock.

In this case we would get 100% deadlock, the same thread can't acquire the same lock twice (unlike in Java for example, where lock is reentrant), lock guard is dropped after evaluating an expression.

You are right, write lock could happen, but in this case we will wait for our turn to get a read lock, I don't see how it may cause a deadlock.

@hashmap
Copy link
Contributor

hashmap commented Jun 3, 2020

Reg your last concern I share it, I'd prefer to get a read lock in the beginning to deal with a snapshot of the current_state.

@BurtonQin
Copy link
Contributor Author

Because write() is prior to read().
Thread-A: Call read() and get the first lock.
Thread-B: Call write() and wait for read lock to release.
Thread-A: Call read() but cannot get the second lock because Thread-B is requesting write lock. Stuck here.

@hashmap
Copy link
Contributor

hashmap commented Jun 3, 2020

Sorry, I still don't get why

Thread-B: Call write() and wait for read lock to release.

Thread-A will release (the first) read lock almost immediately, so write will block the second read for a while, but no big deal.

@BurtonQin
Copy link
Contributor Author

The temporary lockguard returned by the first read() will live until the end of the statement, which covers the second read().

@hashmap
Copy link
Contributor

hashmap commented Jun 3, 2020

Ah, it's a struct init, I missed it, you are right. I thought there are 2 different statements.

@hashmap hashmap merged commit 450d235 into mimblewimble:master Jun 3, 2020
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