Support for new Slic ping/pong frame protocol #3348
Conversation
| } | ||
|
|
||
| // The ping task is necessarily waiting for the pong frame if the wait for pong TCS is not completed. | ||
| Debug.Assert(!_pingTask.IsCompleted); |
There was a problem hiding this comment.
This looks bogus to me: when the ping task is canceled by DisposeAsync, the _waitForPongFrameTcs is still alive and running.
| (ref SliceDecoder decoder) => new PongBody(ref decoder), | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| _waitForPongFrameTcs.SetResult(pongBody); |
There was a problem hiding this comment.
This is a synchronous continuation in the "read loop task".
It's dubious to do all this work this in this critical read loop task.
There was a problem hiding this comment.
The continuation is just a payload comparison. But fine, I've changed it to run the continuation asynchronously.
There was a problem hiding this comment.
I've re-worked the code to no longer use a TCS.
| // TODO: KeepAlive is called every IdleTimeout / 2 if it's not deferred by a write. Instead, we could | ||
| // consider disabling the KeepAlive timer until the pong frame is received. | ||
|
|
||
| // If not waiting for a pong frame (_pongIsPending=0), send a new ping frame. |
There was a problem hiding this comment.
So you fixed the TODO above?
There was a problem hiding this comment.
No, I added it to get feedback on it. Opened #3354 to discuss if we want to fix it and how to fix it.
| WriteConnectionFrame(FrameType.Pong, new PongBody(pingBody.Payload).Encode); | ||
| } | ||
|
|
||
| async Task ReadPongFrameAsync(int size, CancellationToken cancellationToken) |
There was a problem hiding this comment.
When this method returns a faulted Task, the "read loop task" fails and the Slic connection is aborted?
There was a problem hiding this comment.
Yes, the read loop task awaits ReadPongFrameAsync so it will fail.
However, it doesn't abort the connection directly. Instead, AcceptStreamAsync fails and triggers the protocol connection ShutdownRequested task completion. The client connection or connection pool will start the shutdown sequence and dispose the protocol connection that will dispose the transport connection.
This PR implements the proposal from #3273