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

Fix Swarm<T>.BroadcastTxAsync() an exception from IStore on some race conditions #476

Merged
merged 6 commits into from
Aug 30, 2019

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Aug 29, 2019

I believe this fixes #352, but I haven't found any concise way to reproduce this in unit tests…

@dahlia dahlia added the bug Something isn't working label Aug 29, 2019
@dahlia dahlia self-assigned this Aug 29, 2019
earlbread
earlbread previously approved these changes Aug 29, 2019
moreal
moreal previously approved these changes Aug 29, 2019
@codecov
Copy link

codecov bot commented Aug 29, 2019

Codecov Report

Merging #476 into 0.5-maintenance will decrease coverage by 0.03%.
The diff coverage is 97.1%.

@@                 Coverage Diff                 @@
##           0.5-maintenance     #476      +/-   ##
===================================================
- Coverage            87.98%   87.95%   -0.04%     
===================================================
  Files                  195      195              
  Lines                13728    13770      +42     
===================================================
+ Hits                 12079    12111      +32     
- Misses                1337     1346       +9     
- Partials               312      313       +1
Impacted Files Coverage Δ
Libplanet.Stun/Stun/TurnClient.cs 0% <0%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 94.51% <100%> (ø) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 92.66% <98.5%> (+0.21%) ⬆️
Libplanet/Crypto/PrivateKey.cs 85.34% <0%> (-0.87%) ⬇️
Libplanet/Net/Swarm.cs 78.71% <0%> (-0.52%) ⬇️

@dahlia dahlia dismissed stale reviews from moreal and earlbread via 24ab579 August 29, 2019 07:01
@longfin
Copy link
Member

longfin commented Aug 29, 2019

It's a little weird because this scenario didn't seem a race condition. (9e66F6faF87109163BFB7cdBfB62685DfbE1Fcea and 925219D488Ee73c42EB308ac178Ea1D6D55288eC are using separated store each other)

FYI I've seen NullReferenceException when LiteDBStore that already disposed of has been accessed.

@dahlia
Copy link
Contributor Author

dahlia commented Aug 29, 2019

@longfin Made SwarmTest.Dispose() method to dispose things in the correct order: 3dc86f9.

longfin
longfin previously approved these changes Aug 29, 2019
@dahlia dahlia requested review from earlbread and moreal August 29, 2019 07:52
earlbread
earlbread previously approved these changes Aug 29, 2019
@dahlia dahlia dismissed stale reviews from earlbread and longfin via 4950935 August 29, 2019 08:46
@dahlia dahlia force-pushed the fix-broadcast-tx-async branch from 4950935 to 3c59bc6 Compare August 29, 2019 08:54
@dahlia dahlia force-pushed the fix-broadcast-tx-async branch 6 times, most recently from 0cd8241 to 7d6b1b4 Compare August 30, 2019 05:57
@dahlia dahlia force-pushed the fix-broadcast-tx-async branch 3 times, most recently from 470bfed to 379b8bb Compare August 30, 2019 11:21
@dahlia dahlia force-pushed the fix-broadcast-tx-async branch from 379b8bb to 90043bd Compare August 30, 2019 11:32
@dahlia dahlia requested review from longfin and earlbread August 30, 2019 12:00
@dahlia
Copy link
Contributor Author

dahlia commented Aug 30, 2019

The reason Linux/Mono builds failed was not due to this patch, but PreloadAsyncCancellation() mining fixture blocks take long time on Linux/Mono (for unknown reason). I fixed this by making fixture blocks reused.

@dahlia dahlia merged commit ac733a1 into planetarium:0.5-maintenance Aug 30, 2019
@dahlia dahlia mentioned this pull request Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants