[SignalR] Add client return results

[BrennanConroy]

This is the majority of the #5280 work.

There is still some cleanup needed like adding logging and better exceptions/exception messages.
And some additional work needed after this change goes in that I'll list in the issue later.

[davidfowl]

.First.Span? Shouldn't this write the whole thing?

[BrennanConroy]

That's what the assert is for, currently we're always allocating a byte[] to store the data.

[davidfowl]

super nit

Suggested change

	// Review: Technically we could pass the sequence without copying into a new array
	// REVIEW: Technically we could pass the sequence without copying into a new array

[halter73]

We should update this comment now to reflect we're not copying here.

[halter73]

Are we going to update this?

[davidfowl]

cc @JamesNK (JRaw?)

[JamesNK]

I don't think JRaw is useful here.

However, this code could be optimized. Rather than ToString + GetBytes, you could serialize the JToken directly to a MemoryStream then ToArray. Or even get the underlying array from the memory stream, trim it as Memory<bytes> and create a sequence from that.

[BrennanConroy]

Are you suggesting:

var token = JToken.Load(reader);
using var strm = new MemoryStream();
using var writer = new StreamWriter(strm);
using var jsonTextWriter = new JsonTextWriter(writer);
token.WriteTo(jsonTextWriter);
jsonTextWriter.Flush();
writer.Flush();
Memory<byte> buf;
if (strm.TryGetBuffer(out var segment))
{
    buf = segment.Array.AsMemory(segment.Offset, segment.Count);
}
else
{
    buf = strm.ToArray();
}

[davidfowl]

Why are we copying? We should make the caller copy.

[BrennanConroy]

If we can find a clean way to use a pool in this type then we can state in the doc comments that we'll do the copying so they don't need to allocate. But if we decide not to use a pool then we can say callers should copy.

[davidfowl]

I wonder if we need a SupportsClientResults bool so we can throw a nicer error when implementations don't support it. We can check and throw in the ISingleClientProxy

[BrennanConroy]

Not sure why a new API is needed. I updated the error message to:
$"{GetType().Name} does not support client return values." so it's clear what the problem is.

[davidfowl]

Clever!

[davidfowl]

This is non-breaking right?

[BrennanConroy]

Should be

[halter73]

This seems fine as long as it's not being sent all the way to the client.

[BrennanConroy]

So this is being sent all the way to the client and our idea of storing a tuple of (connectionId, invocationId) won't work.

The invocationId needs to be the main source of truth for identifying a specific invoke as it is used by the HubProtocols when querying for the expected type for deserialization. If we stored as a (connectionId, invocationId) then we won't have the connectionId when asking the dictionary for the type. We could in theory iterate over every item and find the invocationId, but then it's possible to have duplicates for IDs coming from different servers.

TLDR; The invocationId needs to be unique across all servers. There are 3 ways to do this, use the connection ID as part of the invocationId (what I did), use Redis to store a key and have all servers accessing and incrementing it (please no), or generate another unique key like we do for connection IDs to represent the invocationId.

[halter73]

I still don't see why any of this would mean the connection id needs to be transmitted back and forth from the client. I could imagine some sort of low-level transport filter that added the client's connection id to the invocation id on the way to the server and strips it on the way out. This wouldn't have to change the internal logic on the server at all.

[BrennanConroy]

The problem is HubProtocol only has the invocationID when asking for the type. If servers don't generate a unique ID across all servers then there could be multiple of the same IDs.

[halter73]

I see. We could pass the IHubProtocol a connection id outside of the payload, but then that'd require changing the interface which we've avoided. Even though the IHubProtocol does need to support RawResult anyway. What happens again if you try using client return values with an IHubProtocol that doesn't support it?

Couldn't we pass in a per-connection, IInvocationBinder that adds the connection id prefix back to the invocation id so the IHubProtocol doesn't need to be aware of it?

[BrennanConroy]

That's an interesting idea! I'll think about it for preview5

[halter73]

So this races with tokenRegistration?.Dispose(); right? Is there any impact other than not cleaning up the registration as soon as we could?

[BrennanConroy]

I can't think of any issues. My biggest issue with this is the closure allocation.

[halter73]

Can we abort the connection and log an error if we're awaiting MaximumParallelInvocationsPerClient client invocations at once even though it's greater than 1? This is a huge footgun. I can see a lot of connections locking up for no apparent reason.

[halter73]

Should we file an issue for this?

[BrennanConroy]

Planning on having a list of known items on the original issue once this is merged.

[halter73]

Do we have a test for returning null as the Task<TResult>?

[BrennanConroy]

You mean Task<object> () => null or object () => null?
Task<object> () => null will null ref on ConfigureAwait

[halter73]

Are we going to update this?

[halter73]

Could a misbehaving server cause this to happen? If so, we should just throw instead of assert.

[BrennanConroy]

Well lots of things can happen if there is a misbehaving server :)

We always generate a unique ID for the invocation so this will never be an issue for our servers right now. If we do collide then it's a big problem.

[halter73]

Since we love new so much, we should add a non-nullable new public string InvocationId { get; } thats non-nullable to CompletionMessage

[halter73]

I think this is fine. What's the alternative?

[BrennanConroy]

Adding yet another API, #40811 (comment), but I think this is fine as is.

[halter73]

Should we file an issue for this?

[halter73]

I see. We could pass the IHubProtocol a connection id outside of the payload, but then that'd require changing the interface which we've avoided. Even though the IHubProtocol does need to support RawResult anyway. What happens again if you try using client return values with an IHubProtocol that doesn't support it?

Couldn't we pass in a per-connection, IInvocationBinder that adds the connection id prefix back to the invocation id so the IHubProtocol doesn't need to be aware of it?