Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
[SignalR] Add client return results #40811
Conversation
| /// <param name="methodName">The name of the hub method to define.</param> | ||
| /// <param name="handler">The handler that will be raised when the hub method is invoked.</param> | ||
| /// <returns>A subscription that can be disposed to unsubscribe from the hub method.</returns> | ||
| public static IDisposable On<T1, T2, T3, T4, T5, T6, T7, T8, TResult>(this HubConnection hubConnection, string methodName, Func<T1, T2, T3, T4, T5, T6, T7, T8, Task<TResult>> handler) |
There was a problem hiding this comment.
Just copied what we had for the non-result On methods.
| throw new ArgumentNullException(nameof(hubConnection)); | ||
| } | ||
|
|
||
| return hubConnection.On(methodName, |
src/SignalR/common/Protocols.Json/src/Protocol/JsonHubProtocol.cs
Outdated
Show resolved
Hide resolved
| if (resultType == typeof(RawResult)) | ||
| { | ||
| Debug.Assert(((RawResult)message.Result).RawSerializedData.IsSingleSegment); | ||
| writer.WriteRawValue(((RawResult)message.Result).RawSerializedData.First.Span, skipInputValidation: true); |
There was a problem hiding this comment.
.First.Span? Shouldn't this write the whole thing?
There was a problem hiding this comment.
That's what the assert is for, currently we're always allocating a byte[] to store the data.
| reader.Skip(); | ||
| var end = reader.BytesConsumed; | ||
| var sequence = input.Slice(start, end - start); | ||
| // Review: Technically we could pass the sequence without copying into a new array |
There was a problem hiding this comment.
super nit
| // 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 |
| var token = JToken.Load(reader); | ||
| var str = token.ToString(Formatting.None); | ||
| result = new RawResult(new ReadOnlySequence<byte>(Encoding.UTF8.GetBytes(str))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
}
src/SignalR/common/Protocols.NewtonsoftJson/src/Protocol/NewtonsoftJsonHubProtocol.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public (Type Type, string ConnectionId, Func<CompletionMessage, Task> Completion) RemoveInvocation(string invocationId) | ||
| { | ||
| _pendingInvocations.Remove(invocationId, out var item); |
There was a problem hiding this comment.
TryRemove?
| _pendingInvocations.Remove(invocationId, out var item); | |
| _pendingInvocations.TryRemove(invocationId, out var item); |
There was a problem hiding this comment.
Remove and TryRemove are the same thing in ConcurrentDictionary, do you prefer TryRemove?
| { | ||
| if (item.ConnectionId != connectionId) | ||
| { | ||
| throw new Exception("wrong ID"); |
There was a problem hiding this comment.
TODO: Fix the exception and message?
| public RawResult(ReadOnlySequence<byte> rawBytes) | ||
| { | ||
| // Review: If we want to use an ArrayPool we would need some sort of release mechanism | ||
| RawSerializedData = new ReadOnlySequence<byte>(rawBytes.ToArray()); |
There was a problem hiding this comment.
Why are we copying? We should make the caller copy.
There was a problem hiding this comment.
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.
| /// <param name="args">The invocation arguments.</param> | ||
| /// <param name="cancellationToken">The token to monitor for cancellation requests. The default value is <see cref="CancellationToken.None" />.</param> | ||
| /// <returns>The response from the connection.</returns> | ||
| public virtual Task<T> InvokeConnectionAsync<T>(string connectionId, string methodName, object?[] args, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
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
| // TODO: this relies on the lifetime manager keeping state for the return type after deserializing the message, is that ok? | ||
| // InvocationId is always required on CompletionMessage, it's nullable because of the base type | ||
| else if (_hubLifetimeManager.TryGetReturnType(completionMessage.InvocationId!, out _)) |
There was a problem hiding this comment.
Do we need another method on the HubLifetimeManager here?
| _parallelEnabled = parallelEnabled; | ||
| } | ||
|
|
||
| private class NotParallelSingleClientProxy : ISingleClientProxy |
| { | ||
| // Written as a MessagePack 'arr' containing at least these items: | ||
| // * A MessagePack 'arr' of 'str's representing the excluded ids | ||
| // * [The output of WriteSerializedHubMessage, which is an 'arr'] | ||
| // For invocations expecting a result |
src/SignalR/server/StackExchangeRedis/src/RedisHubLifetimeManager.cs
Outdated
Show resolved
Hide resolved
src/SignalR/server/StackExchangeRedis/src/RedisHubLifetimeManager.cs
Outdated
Show resolved
Hide resolved
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.