Conversation
chm-diederichs
changed the title
HDegroote
left a comment
HDegroote
left a comment
There was a problem hiding this comment.
Left some questions, but I don't fully get the context of the PR and the consequences of refusing connections when we're not downloading, so needs a review by someone else too
|
|
||
| if (this.destroyed) return | ||
| if (downloading || this._notDownloadingLinger === 0) { | ||
| if (downloading || this._notDownloadingLinger === 0 || !this.peers.length) { |
There was a problem hiding this comment.
Sanity check: isn't the point of having this._notDownloadingLinger that we postpone switching off the downloading a bit? We seem to override that check when this.peers.length === 0
| } | ||
|
|
||
| _makePeer (protomux) { | ||
| if (!this.downloading) return false |
There was a problem hiding this comment.
This needs some thought, because I think it could be dangerous. Doesn't this mean we will never open replication sessions to peers if we're not downloading the hypercore ourselves?
So the underlying assumption is: we always have this.downloading set to true when we open a connection and actually want to replicate.
For example, this assumes there's no path where we open a connection to a peer while this.downloading === false but set it to true later.
(not saying this isn't correct, it just rings some alarm bells of 'potentially dangerous'. The fact that all tests pass does seem like a good sign)
There was a problem hiding this comment.
ya true, we should add a test that checks if an inactive peer can replicate with an active peer if we dont
4 participants
No description provided.