Skip to content

libs/bits: inline defer and change order of mutexes#5187

Merged
melekes merged 3 commits intomasterfrom
anton/3217-deadlocks
Aug 26, 2020
Merged

libs/bits: inline defer and change order of mutexes#5187
melekes merged 3 commits intomasterfrom
anton/3217-deadlocks

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Jul 31, 2020

Closes #3217

@auto-comment
Copy link

auto-comment bot commented Jul 31, 2020

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

a) it's not called outside
b) the reason for exposing it in the first place is unclear
c) Stop already exist if someone from outside wants to stop the client
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #5187 into master will increase coverage by 1.59%.
The diff coverage is 63.48%.

@@            Coverage Diff             @@
##           master    #5187      +/-   ##
==========================================
+ Coverage   60.83%   62.43%   +1.59%     
==========================================
  Files         203      258      +55     
  Lines       20526    27129    +6603     
==========================================
+ Hits        12488    16937    +4449     
- Misses       6961     8718    +1757     
- Partials     1077     1474     +397     
Impacted Files Coverage Δ
blockchain/v2/processor_context.go 63.15% <0.00%> (ø)
cmd/tendermint/commands/run_node.go 0.00% <0.00%> (ø)
config/config.go 77.59% <ø> (ø)
config/toml.go 65.95% <ø> (ø)
consensus/replay_stubs.go 59.57% <0.00%> (-1.30%) ⬇️
crypto/crypto.go 0.00% <ø> (ø)
crypto/sr25519/pubkey.go 44.82% <ø> (ø)
evidence/pool.go 66.49% <ø> (-0.40%) ⬇️
evidence/reactor.go 61.20% <ø> (+0.52%) ⬆️
evidence/services.go 66.66% <ø> (ø)
... and 124 more

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Aug 11, 2020
@erikgrinaker erikgrinaker removed the stale for use by stalebot label Aug 11, 2020
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Aug 22, 2020
@tac0turtle tac0turtle removed the stale for use by stalebot label Aug 22, 2020
@melekes melekes marked this pull request as ready for review August 26, 2020 05:34
@melekes melekes requested a review from tessr as a code owner August 26, 2020 05:34
@melekes melekes self-assigned this Aug 26, 2020
@erikgrinaker
Copy link
Contributor

Good catch. The unlocks were probably both deferred earlier, and defers are executed in reverse order, so someone probably just blindly copied them into a function in the written order.

Are the socket client changes supposed to be here? They seem unrelated to the bit array changes.

@melekes
Copy link
Contributor Author

melekes commented Aug 26, 2020

Are the socket client changes supposed to be here? They seem unrelated to the bit array changes.

Kinda. I've refactored socket client while investigating #3217 (comment)

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Ok, looks good in any case!

@melekes melekes merged commit 4827ed1 into master Aug 26, 2020
@melekes melekes deleted the anton/3217-deadlocks branch August 26, 2020 09:18
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.

Potential deadlocks as reported by go-deadlock

3 participants