search

Shmew / Fable.SignalR

A functional type-safe wrapper for SignalR and Fable.
https://shmew.github.io/Fable.SignalR/
MIT License
90 stars 17 forks source link

Support cancellation of streams created with StreamFrom in .NET client #32

Open kentcb opened 3 years ago

kentcb commented 3 years ago

This relates to the .NET client - I am not sure if it affects other clients.

Is your feature request related to a problem?

IHubConnection<...> has a StreamFrom that accepts a CancellationToken and forwards onto the SignalR hub's StreamAsync method, but HubConnection<...>.StreamFrom does not likewise accept a CancellationToken. This means clients cannot cancel streams.

Describe the solution you'd like

Cancellation should be exposed to clients, allowing them to proactively cancel a stream.

Describe alternatives you've considered

I've hacked this into my own copy of the code to prove it would work how I'd expect (how a raw SignalR stream works, basically). It required changes to both client and server.

kentcb commented 3 years ago

Hi @Shmew. Would you accept a PR that addresses this? I guess the biggest concern would be that it's a breaking change as the function signature for "stream from" would need to change.

Shmew commented 3 years ago

Hi @kentcb,

Yeah I'd accept a PR for this. I think it can be made non-breaking as it could be an optional parameter.

kentcb commented 3 years ago

@Shmew

Just looking at this now and realized I conflated two separate cancellation problems:

  1. .NET clients being able to cancel a stream established with HubConnection<...>.StreamFrom. This one is easy and non-breaking.
  2. .NET server implementation being able to detect the cancellation of a stream. The streamTo that gets passed into AddSignalR does not currently include a CT in its signature, meaning the server has no idea if the client has cancelled. This will be a breaking change, but as far as I can tell it is absolutely critical because otherwise streams could remain open indefinitely on the server.

I'll create a separate issue for point 2 because I believe they're independently addressable.

kentcb commented 3 years ago

Actually, the second issue is more subtle. There is a CT buried away in FableHub<...>.Context.ConnectionAborted, but it is only triggered if - as the name suggests - the connection itself is aborted. It isn't triggered if the client cancels the token passed in when establishing the stream, which means streams could be left open for a long time if the client is long-lived. Anyway, will continue to investigate and create a separate issue once I have my head around it.