Fixed icerpc protocol connection shutdown #2289
Conversation
| { | ||
| throw new IceRpcException(IceRpcError.OperationAborted, exception); | ||
| // Expected if the peer closed the connection first and AcceptStreamAsync closed the connection. |
There was a problem hiding this comment.
I find the comment and the code ultra confusing:
- AcceptStreamAsync is a method on the transport connection. Why would it close the connection??
- Having TransportConnection.CloseAsync and CloseAsync with totally different semantics (CloseAsync calls _transportConnection.DisposeAsync, not CloseAsync) is not helpful
There was a problem hiding this comment.
Fixed the comment to:
// Expected if the peer closed the connection first and the accept request task closed the connection.
Having TransportConnection.CloseAsync and CloseAsync with totally different semantics (CloseAsync calls _transportConnection.DisposeAsync, not CloseAsync) is not helpful.
Ok, I give up 🙂.
Is the following OK?
/// <summary>Marks the protocol connection as closed, disposes the transport connection and cancels
/// pending dispatches and invocations.</summary>
private async Task DisposeTransportAsync(string? message = null, Exception? exception = null)
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Expected if the peer closed the connection first and the accept request task closed the connection. |
There was a problem hiding this comment.
Really: the accept requests loop disposed the transport connection.
| { | ||
| throw new IceRpcException(IceRpcError.OperationAborted, exception); | ||
| // Expected if the peer closed the connection first and the accept request task closed the connection. |
There was a problem hiding this comment.
Really: the "accept requests loop" (or accept requests task) disposed the transport connection while this CloseAsync was in progress.
And presumably, transportConnection.DisposeAsync does not wait for an outstanding CloseAsync to complete: it aborts it. Is this the correct semantics for multiplexed connection?
See also dotnet/runtime#78641 (comment)
There was a problem hiding this comment.
Added comment here #2225 (comment)
This PR fixes the IceRpc protocol connection shutdown to ignore failures from the
CloseAsynccall due to a potentialtransportConnection.DisposeAsync()call.