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

blockchain: Avoid deadlock with additional locking #156

Closed
wants to merge 2 commits into from
Closed

blockchain: Avoid deadlock with additional locking #156

wants to merge 2 commits into from

Conversation

johnsonjh
Copy link

@johnsonjh johnsonjh commented Nov 3, 2020

Found via race detector

@johnsonjh
Copy link
Author

The Travis failure is a false positive which is fixed in PR #127

@cjdelisle
Copy link
Member

pleeease can you just make sendNotification async so that

  1. You don't need to lock it
  2. You don't need to drop other locks to invoke a send

@johnsonjh
Copy link
Author

Probably! I really didn't investigate the logic much in this, just made the patch due to the race detector alert. Leave it open for now, I'll see if there is any reason why not to.

@johnsonjh
Copy link
Author

johnsonjh commented Nov 19, 2020

@cjdelisle Not 100% sure if we can - see btcsuite#1563 for the kind of problems mainline has and we don't, and the behavior during deep/extended reorg is a bit of complex. Also btcsuite#1492 and btcsuite#1660, etc. GCash guys have similar changes to what I came up with at gcash/bchd#308 - I'd sort of feel better with this in place until I can get the test harness fixed and can run full regtestnet with the race detector on, and also because I know we had some extended reorg on the chain the other day and my node with these patches made it through and some of the gridnodes running older software crashed ... but recovered after restart.

Fixing up the test harness is going to high priority for me, so I can more easily test these edge cases, especially for some of the more invasive changes I have in testing like the UDP FBT, etc.

@johnsonjh
Copy link
Author

Merge made by the 'recursive' strategy.
 blockchain/accept.go |  2 ++
 blockchain/chain.go  | 17 ++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

@johnsonjh
Copy link
Author

  • NOTE: Applied to WIP rebase branch successfully.

@johnsonjh johnsonjh closed this Dec 28, 2020
@johnsonjh johnsonjh deleted the blocklock branch December 28, 2020 23:50
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