Fix client invocation bug with multiple endpoints #2154
Conversation
xingsy97
force-pushed
the
siy/fix-invocation-bug
branch
from
427b44c to
0f8a61e
Compare
xingsy97
force-pushed
the
siy/fix-invocation-bug
branch
from
6f3059e to
9431689
Compare
src/Microsoft.Azure.SignalR.Common/ServiceConnections/MultiEndpointMessageWriter.cs
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR/ClientInvocation/CallerClientResultsManager.cs
Show resolved
Hide resolved
| if (serviceMessage is ClientInvocationMessage invocationMessage) | ||
| { | ||
| // Accroding to target endpoints in method `WriteMultiEndpointMessageAsync` | ||
| _clientInvocationManager.Caller.SetAckNumber(invocationMessage.InvocationId, TargetEndpoints.Length); |
There was a problem hiding this comment.
when TargetEndpoints.Length is 0, the result is OK?
There was a problem hiding this comment.
class AckHandler could handle such condition correctly. Refer to its method SetExpectedCount
There was a problem hiding this comment.
xingsy97
force-pushed
the
siy/fix-invocation-bug
branch
from
075f8ed to
68b7aa7
Compare
| if (serviceMessage is ClientInvocationMessage invocationMessage) | ||
| { | ||
| // Accroding to target endpoints in method `WriteMultiEndpointMessageAsync` | ||
| _clientInvocationManager.Caller.SetAckNumber(invocationMessage.InvocationId, TargetEndpoints.Length); |
There was a problem hiding this comment.
do we still need to SetAck if Length == 0?
| // The ack number of invocation will be set inside `WriteAsync`. So adding invocation should be first. | ||
| var task = _clientInvocationManager.Caller.AddInvocation<T>(_hub, connectionId, invocationId, cancellationToken); | ||
| await WriteAsync(message); | ||
| if (ServiceConnectionContainer is not MultiEndpointServiceConnectionContainer) |
There was a problem hiding this comment.
sounds tricky, a ServiceLifttimeManager should not have knowledge of what the container type is, it looks like a strong assumption and is fragile. What if we later change to use other container?
| if (ServiceConnectionContainer is not MultiEndpointServiceConnectionContainer) | ||
| { | ||
| // `WriteAsync` in test class `TestServiceConnectionHandler` does not set ack number. Set the number manually. | ||
| _clientInvocationManager.Caller.SetAckNumber(invocationId, 1); |
There was a problem hiding this comment.
we don't want such logic spread into multiple layers, choose one place for such logic
| var invocationId = clientInvocationManager.Caller.GenerateInvocationId(TestConnectionIds[0]); | ||
| var cts = new CancellationTokenSource(); | ||
| var task = clientInvocationManager.Caller.AddInvocation<string>("TestHub", TestConnectionIds[0], invocationId, cts.Token); | ||
| clientInvocationManager.Caller.SetAckNumber(invocationId, 1); |
There was a problem hiding this comment.
all the test set acknumber manually? how we test if this SetAckNumber logic works well with invocation?
|
I am not sure if I like this approach. How about having WriteAsync return the successful write count? @vwxyzh any thoughts? |
3 participants
Summary of the changes (Less than 80 chars)