Bugfix/issue 277 blocking push block #333
Conversation
Queue has been added for requests, processing is in NextBlockCore Fix stratisproject#277
| @@ -190,7 +196,8 @@ public LookaheadBlockPuller(ConcurrentChain chain, IConnectionManager connection | |||
| public void SetLocation(ChainedBlock tip) | |||
| { | |||
| Guard.NotNull(tip, nameof(tip)); | |||
| this.location = tip; | |||
| lock (this.locationLock) | |||
There was a problem hiding this comment.
This is the rare cases where I prefer brackets over a one like lock.
There was a problem hiding this comment.
And I agree with you, I was just testing the grounds. I don't like it either. Will fix.
| } | ||
|
|
||
| this.logger.LogTrace("(-)"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds requests to download blocks to the queue. |
There was a problem hiding this comment.
You can make this comment more explanatory? (what was the doc code for sentence?)
| } | ||
| else | ||
| { | ||
| // The buffer is either full, so we do not want to ask for more blocks. |
There was a problem hiding this comment.
Ah by the way what happens on a reorg? do we clean all the lists?
There was a problem hiding this comment.
We do have issues with reorgs in all code, we do have an issue for a memory leak somewhere but I believe there will be more issues. The good thing here is that it seems that reorg will just cause requested blocks from different chain to be downloaded. So they will arrive earlier then they would otherwise (if we cleaned the list). So there seems to be not even a memory leak in this queue in case of reorg. So maybe there is no problem to solve here. But again, I think there are problems with reorgs and many yet to be uncovered.
There was a problem hiding this comment.
Ah yeah this list will just clear itself true so no issue here.
There are issue with reorgs true but if we keep proper separation then the issues will be contained to each feature, so we just solve reorg per feature.
In the puller case we still need to clean the download collections on reorg.
There was a problem hiding this comment.
Let's create discussion issue for reorgs elsewhere - #337
| } | ||
|
|
||
| WaitHandle.WaitAny(new[] { this.pushed, cancellationToken.WaitHandle }, WaitNextBlockRoundTimeMs); | ||
| while (!cancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
I see what you mean by complicating the logic
There was a problem hiding this comment.
The weirdest part is that in the same method - NBC - you set this.consumed and you also wait for this.consumed AND you expect only one thread to exist in the node that executes NBC. That is the weird part. But due to the logic of NC that it actually calls AskBlocks and because AskBlocks then calls ProcessQueue it actually works. But is very nontrivial to grasp. The original idea with the thread would separate the logic and the design would be very clean, but yes, it would cost the thread :)
| /// <exception cref="InvalidOperationException">Thrown in case of data inconsistency between synchronized structures, which should never happen.</exception> | ||
| internal void ReleaseAllPeerDownloadTaskAssignments(BlockPullerBehavior peer) | ||
| internal void ReleaseAllPeerDownloadTaskAssignments(BlockPullerBehavior peer, bool peerDisconnected) |
There was a problem hiding this comment.
I would suggest to name it disconnectPeer.
If set to true the peer needs to be is set to a disconnected state and should be prevented from being assigned additional work.
|
OK, should be ready to go. |
Add "join" channel API
Inversion of logic of enforcing
LBP.MaxBufferedSize- blocks are not asked anymore if it is expected that they would violate the limit.Fix #277
Fix #327