Canceled client result can result in connection being closed

[BrennanConroy]

Using the new client results feature in SignalR, if you pass in a cancellation token to InvokeAsync and cancel it, the server-side will properly clean-up the call, but the client-side will still send a result it produces one which will result in the HubProtocol serializer throwing when trying to resolve the type to deserialize the response to (as the server-side doesn't know about the invocation anymore). When the HubProtocol throws the connection is closed because we have to assume the connections pipe is no longer usable (partial reads etc.).

aspnetcore/src/SignalR/server/Core/src/Internal/HubConnectionBinder.cs

Lines 24 to 30 in bf02bf2

	public Type GetReturnType(string invocationId)
	{
	if (_hubLifetimeManager.TryGetReturnType(invocationId, out var type))
	{
	return type;
	}
	throw new InvalidOperationException($"Unknown invocation ID '{invocationId}'.");

[BrennanConroy]

In order to try to promote users to think about timeouts/cancellations when invoking methods on the client from the server, we want to force the user to pass in a CancellationToken, so the API change is to remove the default value from appropriate APIs.

interface ISingleClientProxy
{
-    Task<T> InvokeCoreAsync<T>(string method, object?[] args, CancellationToken cancellationToken = default(CancellationToken));
+    Task<T> InvokeCoreAsync<T>(string method, object?[] args, CancellationToken cancellationToken);
}

static class ClientProxyExtensions
{
-    static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, CancellationToken cancellationToken = default(System.Threading.CancellationToken));
+    static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, CancellationToken cancellationToken);

-    static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, object? arg1, CancellationToken cancellationToken = default(CancellationToken));
+    static Task<T> InvokeAsync<T>(this ISingleClientProxy clientProxy, string method, object? arg1, CancellationToken cancellationToken);

// through 10 args
}

class HubLifetimeManager<THub>
{
-    virtual Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default(CancellationToken));
+    virtual Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken);
}

class DefaultHubLifetimeManager<THub>
{
-    Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default(System.Threading.CancellationToken));
+    Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken);
}

class RedisHubLifetimeManager<THub>
{
-    override Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default(System.Threading.CancellationToken));
+    override Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken);
}

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

I think this API makes sense. APIs like SendAsync which use cancellationToken = default, but the cancellation only applies to connection backpressure. In that case a completely unresponsive client would be timed out by retransmit limits or min rate limits.

In the case of these InvokeAsync APIs, the client may still be responsive at the connection level and avoid any automatic timeout if there's a bug in the client application that causes it not to return a value in a timely matter.

I think forcing people manually pass in a CancellationToken is a good idea. Hopefully it will make developers at least think about the need for app-level timeouts. If someone really wants to wait indefinitely, it's not hard to pass in CancellationToken.None. It's more self-documenting that way. Any objections?

[halter73]

API Approved!