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

Make sure we re-acquire lock if a task throws #6565

Merged
merged 1 commit into from Aug 19, 2015
Merged

Make sure we re-acquire lock if a task throws #6565

merged 1 commit into from Aug 19, 2015

Conversation

casey
Copy link
Contributor

@casey casey commented Aug 17, 2015

This fixes #6394

Pretty simple, it just makes sure to re-acquire the lock in case f throws, so that we don't touch nThreadsServicingQueue without the lock. The empty throw statement re-raises the current exception.

@sipa
Copy link
Member

sipa commented Aug 17, 2015

utACK.

@dcousens
Copy link
Contributor

dcousens commented Aug 18, 2015

Couldn't this be done using a scope bound, RAII locking mechanism instead?

utACK

@casey
Copy link
Contributor Author

casey commented Aug 18, 2015

@dcousens: Oh, hey, it looks like there actually is a reverse lock: http://www.boost.org/doc/libs/1_55_0/doc/html/thread/synchronization.html#thread.synchronization.other_locks.reverse_lock

I'll reimplement tomorrow, should be much cleaner.

@sipa
Copy link
Member

sipa commented Aug 18, 2015

@casey
Copy link
Contributor Author

casey commented Aug 18, 2015

Re-wrote it with a reverse_lock, it's much nicer now

@sipa
Copy link
Member

sipa commented Aug 18, 2015

@dcousens
Copy link
Contributor

dcousens commented Aug 18, 2015

utACK, good find @casey

@laanwj
Copy link
Member

laanwj commented Aug 19, 2015

/me really likes this solution. An inverse lock :-)
utACK.

@laanwj laanwj merged commit fb08d92 into bitcoin:master Aug 19, 2015
1 check passed
laanwj added a commit that referenced this issue Aug 19, 2015
fb08d92 Make sure we re-acquire lock if a task throws (Casey Rodarmor)
@laanwj laanwj added the Bug label Aug 19, 2015
@theuni
Copy link
Member

theuni commented Aug 20, 2015

boost::reverse_lock is new as of 1.50 :(

We either need to set that as the minimum version required or work out a different fix here.

@casey
Copy link
Contributor Author

casey commented Aug 20, 2015

The reverse lock implementation is very simple. If we can't bump the minimum required boost version to 1.50, I could copy pasta it in.

Alternatively we could just go back to the first implementation, which, although ugly, works fine.

@dcousens
Copy link
Contributor

dcousens commented Aug 20, 2015

I see no reason why we wouldn't be able to bump?

@pstratem
Copy link
Contributor

pstratem commented Aug 30, 2015

Debian wheezy is 1.49 please do not bump the required version.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Aug 30, 2015

Also ran into the same problem like @pstratem. I solved it with a dist upgrade to jessie... which is somehow not ideal. Support for boost 1.49 would be good but not urgently necessary.

@dcousens
Copy link
Contributor

dcousens commented Aug 30, 2015

Boost 1.49 was released on February 24th, 2012.
Boost 1.50 was released on June 28th, 2012.
Debian 7.8 was released on January 10th, 2015.

Why the lag on a 4-month older package?

@casey
Copy link
Contributor Author

casey commented Aug 31, 2015

Opened PR #6608, to revert and fix without a boost dependency.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants