[SignalR] Pass new App to Transport Pipe to App layer #49650
Conversation
BrennanConroy
requested review from
Tratcher,
JamesNK and
mgravell
as code owners
BrennanConroy
force-pushed
the
brecon/noex
branch
from
8f50777 to
c0fd980
Compare
| @@ -579,6 +579,13 @@ private async Task StopAsyncCore(bool disposing) | |||
| TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.OnlyOnFaulted, | |||
| TaskScheduler.Default); | |||
| } | |||
|
|
|||
| #pragma warning disable CA2252 // This API requires opting into preview features | |||
There was a problem hiding this comment.
Does this mean that if we do make API changes to preview features in 9.0, it could make things break harder when mixing 8.0 and 9.0 packages?
There was a problem hiding this comment.
So like the following examples:
- If you used 8.0 client on the server that targets 9.0
- If you use Connection.Abstractions 9.0 with 8.0 client
Yeah that does seem bad :/
Generally we say "use the same version packages" since we ship everything at the same time. But it's sort of a cop-out.
| public void DisableReconnect() | ||
| #pragma warning restore CA2252 // This API requires opting into preview features | ||
| { | ||
| _useAck = false; |
There was a problem hiding this comment.
Doesn't it feel like this should be synchronized? Same with the variant in HttpConnectionContext. What if we fire the notifyOnReconnect event way after DisableReconnect() returns? Potentially after the transport and/or connection have fully closed? Could the client start reconnecting after it has already sent a close message over the previous WebSocket due to StopAsync()?
| try | ||
| { | ||
| var reconnectTask = connection.NotifyOnReconnect?.Invoke(connection.Transport.Output) ?? Task.CompletedTask; | ||
| await reconnectTask; |
There was a problem hiding this comment.
Do we really want to await resending everything before disabling the request body timeout? It seems like that could cause us to close healthy connections. Is there any possible benefit to awaiting this task immediately rather than putting it in the WhenAny?
The same goes on the client side even though there's no IHttpRequestTimeoutFeature. Why would the implementor of NotifyOnReconnect ever want to delay execution of anything at the Http.Connections layer? What's the reconnectTask for? I assumed it was just better error reporting. @davidfowl
There was a problem hiding this comment.
Is there any possible benefit to awaiting this task immediately rather than putting it in the WhenAny?
Well the WhenAny is for when the connection is closing, either due to the transport detecting a disconnect or the application completing. So we'd still want to wait for the reconnect task separately. But we should put that after the IHttpRequestTimeoutFeature.
| /// <summary> | ||
| /// Called when a connection reconnects. The new <see cref="PipeWriter"/> that application code should write to is passed in. | ||
| /// </summary> | ||
| public void OnReconnected(Func<PipeWriter, Task> notifyOnReconnect); |
There was a problem hiding this comment.
| public void OnReconnected(Func<PipeWriter, Task> notifyOnReconnect); | |
| public void OnReconnected(Func<PipeWriter, Task> callback); |
src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs
Outdated
Show resolved
Hide resolved
BrennanConroy
requested review from
wtgodbe,
a team,
captainsafia and
mkArtakMSFT
as code owners
BrennanConroy
force-pushed
the
brecon/noex
branch
from
027821f to
4520c67
Compare
BrennanConroy
added
the
Servicing-consider
|
Hi @BrennanConroy. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
BrennanConroy
added
Servicing-approved
|
Hi @BrennanConroy. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed. |
|
This came in before 5 and will get merged, but I'm waiting for a build to succeed after the latest merge conflict resolution. |
ghost
added this to the
Pass new 'App to Transport Pipe' to 'App layer'
Removes the use of
ConnectionResetExceptionto let the app layer know the connection is being replaced. Instead we pass the newPipeto the App layer so it knows when a new underlying connection is present.Also, adds
DisableReconnectso the app layer can let the connection layer know that new connections shouldn't be allowed anymore.