Add core.remoteContiguousLength & remote-contiguous-length event
#743
Conversation
To save messages being sent by all peers, the existing behavior of sending a `range` request in `broadcastRange()` only when fully contiguous is being kept. This means the `.remoteContiguousLength` and the `remote-contiguous-length` event only update / fire when the remote peer is fully contiguous and not when partially contiguous.
This emphasizes that the variable is true only when it is fully contiguous, ie having the full range of blocks. This is in contrast to being partially contiguous.
Includes note that it is only updated when the peer thinks it is fully contiguous (aka non-sparse).
Now allows appends after truncating to fire remote contig event.
To support holepunchto/hypercore#743 persisting `remoteContiguousLength`.
Fixes the `remote contiguous length - persists` test assuming storage support.
|
Currently depends on holepunchto/hypercore-storage#92 for persistence test. |
|
|
||
| const i = Math.floor(start / DEFAULT_SEGMENT_SIZE) | ||
| const contig = this.core.header.hints.contiguousLength === this.core.state.length | ||
| const fullyContig = this.core.header.hints.contiguousLength === this.core.state.length |
There was a problem hiding this comment.
(detail) unneeded renaming of the variable?
There was a problem hiding this comment.
The renaming was because initially I thought it meant that the local core was contiguous even if partially so. So renamed only to clarify that it is fully contiguous in the sense that it matches the latest length.
But yeah, no functionality change.
Not likely to happen in practice, but just to be safe.
Peers always send `range` requests starting with `0` if they are contiguous. Partial ranges would imply they are not contiguous so shouldn't be able to sparsely update the `remoteContiguousLength`. Because there is no need check the `start` of the range & `core.updateRemoteContiguousLength()` already checks if the `length` is larger, the logic is simplified to only call `updateRemoteContiguousLength()` when updating the `_remoteContiguousLength` for the peer when the range isnt a drop.
Could mask underlying error and its assumed that storage errors at this point should not be caught.
lib/replicator.js
Outdated
| if (length > this._remoteContiguousLength) this._remoteContiguousLength = length | ||
| if (length > this._remoteContiguousLength) { | ||
| this._remoteContiguousLength = length | ||
| this.core.updateRemoteContiguousLength(length) |
There was a problem hiding this comment.
I'm not sure we can let it go uncaught either.
I think either we need a dedicated error handler, but we don't have any standard place to emit the event..
What do you think about changing updateRemoteContiguousLength to be sync and have it set a needsHintFlush flag on the core, then SessionState.flush always checks that flag and when flushed these lines are triggered by calling a _onhintflush hook instead?
There was a problem hiding this comment.
it would mean that remoteContiguousLength may be stale on restart, not sure of the ramifications there
There was a problem hiding this comment.
seems like we aren't bubbling up from the replicator anyways, so can just catch with an error handler that mimics here
There was a problem hiding this comment.
@chm-diederichs so a safetycatch and is the WRITE_FAILED the only error code we want to pause from?
There was a problem hiding this comment.
no I think we should pause on any error, my link was misleading, I meant to refer to just the safetyCatch and the pause
Should test if a reorg will correctly update the remote contiguous length to match.
lib/replicator.js
Outdated
| if (length > this._remoteContiguousLength) this._remoteContiguousLength = length | ||
| if (length > this._remoteContiguousLength) { | ||
| this._remoteContiguousLength = length | ||
| if (this.remoteFork >= this.core.state.fork) { |
There was a problem hiding this comment.
would add this condition to the if (inverse) https://github.com/holepunchto/hypercore/pull/743/files#diff-826e5ed9069c92baa50ec46dde8074a6789b4c90837c5ede0e6b0438e3c34309R718
`core.updateRemoteContiguousLength()` is only called from `onrange` so moving the check doesn't allow unviable lengths through from elsewhere.
* Update schema code gen files Includes formatting & adding the `"strings"` flag to the JSON for enums. * Add `remoteContiguousLength` field to hints To support holepunchto/hypercore#743 persisting `remoteContiguousLength`. * Update setHint test to update remoteContiguousLength
Use ternary instead of a combination of optional chaining and null coalescing.
Add the property
.remoteContiguousLengthfor the maximum known peer contiguous length and theremote-contiguous-lengthevent for when it updates.Added documentation noting the following exceptions: