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

Access blocks and transactions of the chain with lock #583

Merged
merged 4 commits into from
Oct 15, 2019

Conversation

earlbread
Copy link
Contributor

@earlbread earlbread commented Oct 15, 2019

#409 occurs when a task tries to read a block or transaction from BlockChain<T>.Blocks or BlockChain<T>.Transactions while another task is writing it. This fixes #409 by changing blocks and transactions in the BlockChain<T> so that it cannot be accessed without the lock from the outside of the chain.

@earlbread earlbread added the bug Something isn't working label Oct 15, 2019
@earlbread earlbread self-assigned this Oct 15, 2019
/// </param>
/// <exception cref="KeyNotFoundException">Thrown when there is no
/// <see cref="Transaction{T}"/> with a given <paramref name="txId"/>.</exception>
public Transaction<T> this[TxId txId]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of overloading this and making indexer able to return two distinct types, how about declare a method like GetTransaction(TxId)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be better. Fixed.

@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #583 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
- Coverage   90.74%   90.72%   -0.02%     
==========================================
  Files         202      202              
  Lines       15514    15542      +28     
==========================================
+ Hits        14078    14101      +23     
- Misses       1144     1149       +5     
  Partials      292      292
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 84.78% <100%> (ø) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 98.95% <100%> (ø) ⬆️
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.68% <100%> (-0.01%) ⬇️
Libplanet/Blockchain/BlockChain.cs 94.5% <100%> (+0.25%) ⬆️
Libplanet/Store/BaseIndex.cs 20.75% <0%> (-9.44%) ⬇️

limebell
limebell previously approved these changes Oct 15, 2019
moreal
moreal previously approved these changes Oct 15, 2019
longfin
longfin previously approved these changes Oct 15, 2019
@longfin
Copy link
Member

longfin commented Oct 15, 2019

I think that we also need to add a mutex for block synchronization to LiteDBStore. (even in other PR)

dahlia
dahlia previously approved these changes Oct 15, 2019
Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Co-Authored-By: Hong Minhee <hong.minhee@gmail.com>
@earlbread earlbread dismissed stale reviews from dahlia, longfin, and moreal via a0a0ec8 October 15, 2019 12:18
@earlbread earlbread merged commit 24a2e5a into planetarium:master Oct 15, 2019
@earlbread earlbread deleted the fix-blocks branch October 16, 2019 06:49
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.

Bencodex.DecodingException: stream terminates unexpectedly at 0
5 participants