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

Allow server streaming implementations to detect cancellation #34

Open
kentcb opened this issue Sep 15, 2021 · 0 comments
Open

Allow server streaming implementations to detect cancellation #34

kentcb opened this issue Sep 15, 2021 · 0 comments

Comments

@kentcb
Copy link

kentcb commented Sep 15, 2021

Is your feature request related to a problem?

This could reasonably be considered a bug, but I'm submitting as a feature request...

The only CancellationToken available to server stream implementations is the one inside FableHub<...>.HubContext.ConnectionAborted. This CT is provided by SignalR and exposed by Fable.SignalR. However, it is not sufficient for a server stream to base its life time on this CT because it only triggers if the connection itself is severed. It is not triggered in the event of a client explicitly cancelling a stream like this:

let cancellationTokenSource = new CancellationTokenSource();
let stream = hubConnection.StreamAsync<int>("Counter", 10, 500, cancellationTokenSource.Token)

// later
cancellationTokenSource.Cancel() |> ignore

The requirement for that to work is that the hub's streaming method must include a CancellationToken parameter, but Fable.SignalR's hubs do not declare this parameter.

See the docs on streaming for more info.

Describe the solution you'd like

Streaming methods in the various hubs should include a CancellationToken parameter that is then passed into the stream implementation. Unfortunately, this means that the streaming function signature will need to change to include the CT. This will be a breaking change, but unavoidable as far as I can tell.

Describe alternatives you've considered

The only alternative would be to add a property to the FableHub interface, but that would mean:

  1. The property is only relevant to stream implementations. e.g. it could be called StreamCancelled
  2. The property would exist in the FableHub rather than the HubContext, so it would feel a bit weird
  3. The implementation of stream methods would need to create a new object instance with an updated CT before forwarding the call onto the stream implementation
  4. It would be easy for stream implementations to ignore or miss the significance of this CT

To me, it's cleaner and clearer to change the existing function signature, making it clear to implementations that they really should care about that CT and not ignore it.

Additional context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant