New checkpoint request protocol - #97
Conversation
simolus3
left a comment
There was a problem hiding this comment.
I wanted to leave a few early comments in case it helps. As far as the public API is concerned this looks good to me.
|
|
||
| internal class CheckpointStateSignals | ||
| { | ||
| private CheckpointState _state = new CheckpointState.Pending(); |
There was a problem hiding this comment.
It looks like this might not be thread-safe. It's annoying that there is no MutableStateFlow-equivalent, but maybe the Mutex-based implementation from Swift could be easier to port?
|
|
||
| public Task WaitForCheckpointRequestsReady(CancellationToken signal, bool wakeDownloadLoop = true) | ||
| { | ||
| var tcs = new TaskCompletionSource<bool>(); |
There was a problem hiding this comment.
The main reason for this layer of indication in JavaScript was that we otherwise can't cancel waiting for status updates.
ChannelReader does not have that issue, is it possible to structure this method like (pseudo-C#):
var reader = _stateBroadcaster.Subscribe(out var subscriberId);
try {
if (HandleState(_state)) return;
while (!HandleState(reader.WaitToReadAsync(signal))) {}
} finally {
// ... also remove this from HandleState
_stateBroadcaster.Unsubscribe(subscriberId);
}There was a problem hiding this comment.
For some reason I thought WaitToReadAsync(CancellationToken) wasn't available on netstandard2.0 (it is). Probably getting it confused with another cancellable Stream method.
…dard2.0 runtime features, use manual locking in CheckpointStateSignals
Implements the new Checkpoint Requests for .NET, using the other SDK repos as reference (mostly JS and Kotlin).
Note that this uses strings for the checkpoint request Id despite C# having a robust native 64-bit integer type. This is mostly for consistency with other internal APIs and to avoid any accidental public API problems, but this is going to be changed in v1.