Skip to content

Disposing a SlicConnection doesn't do a graceful shutdown #2433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pepone opened this issue Jan 4, 2023 · 2 comments
Closed

Disposing a SlicConnection doesn't do a graceful shutdown #2433

pepone opened this issue Jan 4, 2023 · 2 comments
Labels
question Further information is requested

Comments

@pepone
Copy link
Member

pepone commented Jan 4, 2023

With .NET Quic implementation disposing of the QuicConnection triggers a graceful shutdown

https://github.com/dotnet/runtime/blob/6c0c96ccc775cad4119e2d5d3a1797c4126b4533/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs#L622-L632

https://github.com/microsoft/msquic/blob/main/docs/api/ConnectionShutdown.md

For SlicConnection it just aborts the streams and disposes of the transport connection, but we don't send a close frame

https://github.com/zeroc-ice/icerpc-csharp/blob/5e9dea711b44020093a7f299a18c1099183a7d76/src/IceRpc/Transports/Internal/SlicConnection.cs#L400

I assume this is fine for Slic, because we first stop accepting streams, then abort streams and finally dispose of the transport connection, I just bring this up because is supposed to be like Quic and seems there is a small discrepancy here.

@pepone pepone added the question Further information is requested label Jan 4, 2023
@bentoi
Copy link
Contributor

bentoi commented Jan 4, 2023

Also related to this: dotnet/runtime#78641 and #2225

IMO, it's better for DisposeAsync to be abortive. What's the point of trying to do a graceful, potentially lengthy, connection closure if the peer is misbehaving by sending bogus Slic/IceRpc protocol data (for example)?

I wish Quic didn't have this DefaultCloseErrorCode and worked like other network APIs.

Now if we really want to align Slic with this debatable Quic dispose semantics, we can perform a graceful close from DisposeAsync and add DefaultCloseErrorCode. It won't really change anything as far as the icerpc connection implementation is concerned (except for the message attached to the ConnectionAborted exception/error).

We'll have to study how the Quic connection disposal works if the transport idle timeout kicks in. It's quite possible that the MsQuic connection shutdown is silent in this case. We'll have to do the same.

In any case, I think we have better fish to fry for 0.1.

@pepone
Copy link
Member Author

pepone commented Jan 4, 2023

Closing as dup of #2225

@pepone pepone closed this as completed Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants