Make INetworkConnection Disposable and AsyncDisposable

[bernardnormier]

Update: see first comment below and #1404.

INetworkConnection is currently disposable. I think we should make it async disposable as well.

The semantics of Dispose is Close/Abort immediately and silently with (if needed) a non-specific exception such as ConnectionAbortedException. There is no need for an error code since the closure is silent.

It's already implemented:
https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/Internal/TcpNetworkConnection.cs#L28
https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/Internal/SlicNetworkConnection.cs#L289 (uses ConnectionClosedException though)

The semantics of DisposeAsync is ShutdownAsync = graceful closure.

That's currently not implemented. We have ShutdownAsync but it does not close the connection, it only shuts it down, just like the Sockets and SslStream API. I don't see why we would want ShutdownAsync to only shut down the connection and not close it. This is also inconsistent with our Server and ClientConnection APIs.

Other issues:

• which error code does DisposeAsync give to IMultiplexedNetworkConnection.DisposeAsync?
  see https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/IMultiplexedNetworkConnection.cs#L25

Do we need these error codes? As far I can tell, we only use error code 0:

await _networkConnection.ShutdownAsync(applicationErrorCode: 0, CancellationToken.None).ConfigureAwait(false);

• which cancellation token does DisposeAsync pass to ShutdownAsync?
  see
  https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/IMultiplexedNetworkConnection.cs#L25
  https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/ISimpleNetworkConnection.cs#L20

For ISimpleNetworkConnection, I don't think we need to cancellation token at all, and we can simply replace ShutdownAsync by DisposeAsync.
see https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/Internal/TcpNetworkConnection.cs#L78 (does not use cancellation token)
https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc.Coloc/Transports/Internal/ColocNetworkConnection.cs#L131 (does not use it either).

For IMultiplexedNetworkConnection, it's not clear. We mostly pass CancellationToken.None, but Slic also passes its own token:
https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/Internal/SlicNetworkConnection.cs#L266

The semantics of canceling the cancellation token given to this ShutdownAsync are not clear to me. We give up on waiting for the response from the remote peer?
https://github.com/zeroc-ice/icerpc-csharp/blob/main/src/IceRpc/Transports/Internal/SlicNetworkConnection.cs#L291

It's also odd to give _readCancelSource.Token to ShutdownAsync, which is really a write operation.

If we don't need an error code and cancellation token, we could eliminate ShutdownAsync and have only DisposeAsync.

[bernardnormier]

It's actually inconvenient to make a class or interface disposable and async disposable with 2 different semantics (silent close vs graceful close) as suggested above.

The issue is in an "async context", the compiler will make you use DisposeAsync, when you may want to call Dispose to get the silent non-graceful close semantics.

As a result, we have to chose between:
a) make the class/interface disposable and add a separate Async method for the graceful close (ShutdownAsync)
b) make the class/interface async disposable and add a separate sync method for the silent ungraceful close (Abort or Close)

Say we pick (a). Should ShutdownAsync call Dispose or not?

The disposable contract means the client must call Dispose no matter what, and it's not helpful to have a separate API that calls Dispose for you:

If you call ShutdownAsync, you don't need to call Dispose

This does not help. It makes the API confusing.

So arguably ShutdownAsync should not call Dispose, even if the only thing you can do with a shut down connection is closing it.

[bentoi]

I don't see why we would want ShutdownAsync to only shut down the connection and not close it. This is also inconsistent with our Server and ClientConnection APIs.

That's how most transport APIs are designed (Socket, SslStream, QuicConnection, WebSocket for instance).

Shutdown or ShutdownAsync (or similar... the naming is quite inconsistent here, see my May 6 blog) perform the transport shutdown sequence whereas Dispose is about releasing resources. I find such an API clearer, you always have to call Dispose no matter what you called before. It's more logical to design our transport API along the lines of other transport APIs, it makes implementing other transports easier.

Afaict, in .NET the only API that considers DisposeAsync as a "graceful" is Stream.IO.Stream. The main motivation was to allow this pattern:

using (var stream = ...)
{
}

It's a reasonable choice for an API which is often going to be used with this pattern (which will be the case for ClientConnection or ConnectionPool.

For other APIs which are typically used as a child of another class (the simple network connection of the ice protocol connection), I find separating the responsibility of the "graceful close" from the "resource disposal" clearer.

See also this endless discussion about this for QuicStream: dotnet/runtime#756

Do we need these error codes? As far I can tell, we only use error code 0

As mentioned above, I find it's better to have a separate IMultiplexedNetworkConnection.ShutdownAsync method. The error code is indeed not useful since the icerpc protocol doesn't have any usage for it (for now). We could remove it. We could also remove it from the Slic close frame but it's probably best to keep it to be consistent with Quic.

For the ShutdownAsync cancellation tokens, see my comments on your PRs. I think it depends what guarantees we want to provide to 3rd party transports:

• 1. do we want to guarantee that IceRpc will never call Dispose while there's a pending ReadAsync, WriteAsync or ShutdownAsync?
• 2. or do we require that the Dispose implementation should both cancel pending operations and release ressources (in which case we can just remove the cancellation token from ReadAsync, WriteAsync, ShutdownAsync).

Our implementation of the ice protocol and Slic are using both Dispose and a cancellation token (_readCancelSource.Token) for cancelling pending network operations, that's a bit bogus. If we choose to guarantee 1) we should use a single cancellation source token to all cancellable network operations, otherwise we should remove _readCancelSource (assuming it's only used for network read cancellation...)

[bernardnormier]

Shutdown or ShutdownAsync (or similar... the naming is quite inconsistent here, see my May 6 blog) perform the transport shutdown sequence whereas Dispose is about releasing resources. I find such an API clearer, you always have to call Dispose no matter what you called before. It's more logical to design our transport API along the lines of other transport APIs, it makes implementing other transports easier.

We are in agreement on this point.

Afaict, in .NET the only API that considers DisposeAsync as a "graceful" is Stream.IO.Stream. The main motivation was to allow this pattern:

To be clear, you want to keep DisposeAsync as a graceful close that logically calls ShutdownAsync for our public APIs - Server, ClientConnection, ResumableClientConnection, ConnectionPool ??

(I think we should).

For other APIs which are typically used as a child of another class (the simple network connection of the ice protocol connection), I find separating the responsibility of the "graceful close" from the "resource disposal" clearer.

I think it's ok to separate graceful close and resource-disposal for all network connections. Once await networkConnection.ShutdownAsync() completes, the network connection is shut down (gracefully), but not disposed/closed, and you still need to call Dispose on it.

For IProtocolConnection, IceProtocolConnection and IceRpcProtocolConnection start tasks when dispatching incoming requests and I think it would be incorrect to consider a protocol connection "disposed" while these tasks as still running. Therefore I propose we make IProtocolConnection async disposable, where DisposeAsync waits for these tasks to complete.

Now, we can discuss the semantics of this IProtocolConnection.DisposeAsync:

• it could await a ShutdownAsync with a canceled token before cleaning up other resources (that's my preference since it's what we do for Server and ClientConnection)
• it could alternatively abort the connection, cancel the dispatch source, await the dispatches and then clean up other resource. In this case the only way to get a graceful shutdown would be to call ShutdownAsync explicitly.

[bentoi]

To be clear, you want to keep DisposeAsync as a graceful close that logically calls ShutdownAsync for our public APIs - Server, ClientConnection, ResumableClientConnection, ConnectionPool ??

Yes, that's still fine with me.

I don't really have a preference for the implementation of IProtocolConnection.

One question however: what are the semantics of Abort and do we actually want to keep it?

[bernardnormier]

One question however: what are the semantics of Abort and do we actually want to keep it?

The IProtocolConnection in question is:

    /// <summary>Aborts the connection.</summary>
    /// <param name="exception">The exception that caused the abort. Pending invocations will throw this exception.
    /// </param>
    void Abort(Exception exception);

If we keep it, I think its semantics should be:

• close immediately and silently the underlying network connection
• cancel the dispatch cancellation source to trigger the cancellation of outstanding dispatches

do we actually want to keep it?

I think the question is whether or not we should provide a public Abort API on ClientConnection and Server.
If we do, then we need IProtocolConnection.Abort to implement it. If we don't provide a public Abort API, IProtocolConnection may not need an Abort either.

For Server and connections created by Server, we currently don't have an Abort. The only option is a graceful shutdown.

Maybe we should indeed do the same for ClientConnection and remove ClientConnection.Abort. The implementation of ShutdownAsync already (logically) aborts the connection after the close timeout.
I assume you plan to move the cancel and close timeout ShutdownAsync logic to the protocol connection implementations?

[bentoi]

I assume you plan to move the cancel and close timeout ShutdownAsync logic to the protocol connection implementations?

I'm leaning toward keeping them on ClientConnection and ServerConnection actually. The protocol connection implementations are already quite complicated and these timeouts aren't really protocol specific.

I see the connect timeout as being the equivalent of:

var source = new CancellationTokenSource(timeout);
connection.ConnectAsync(source.Token);

Right now, the shutdown timeout is different however since it aborts the connection. Is that correct though? It would be more logical if it was the same as:

var source = new CancellationTokenSource(timeout);
connection.ShutdownAsync(source.Token);

As for keeping Abort... the only use case I can think of is actually for server-side connections to abort unauthorised connections and free its resources ASAP. It could just be IConnection.Abort(), no need to have such a method on Server.

[bentoi]

Also... Abort could just silently close the underlying network connection if its purpose is only to break "ties" with the peer.

Presumably, an OnClose callback will shutdown the connection once aborted. That's what Server does ... however it's a regular shutdown, not a speedy one. Should the callback be provided with a "bool aborted" parameter to let the server decide if it should do a speedy or non-speedy shutdown? Or have OnAbort and OnShutdown callbacks instead.

[bernardnormier]

Also... Abort could just silently close the underlying network connection if its purpose is only to break "ties" with the peer.
Or have OnAbort and OnShutdown callbacks instead.

I think it's much simpler to make Abort cancel the dispatches. This way, we can keep a single OnClose that does not need to worry about this cancellation token.

[bernardnormier]

I see the connect timeout as being the equivalent of:

I assume that's not the case since connection.ConnectAsync(source.Token); can hang forever.

For me, it should be more the equivalent of:

try
{
    // cancel is the cancellation token from the application
    await connection.ConnectAsync(cancel).WaitAsync(connectTimeout).ConfigureAwait(false);
}
catch (TimeoutException)
{
    connection.Abort(); // give up on this connection attempt
    throw;
}

And I think the same makes sense for shutdown and the shutdown timeout:

try
{
    // cancel is the cancellation token from the application
    await connection.ShutdownAsync(cancel).WaitAsync(shutdownTimeout).ConfigureAwait(false);
}
catch (TimeoutException)
{
    connection.Abort(); // give up on this shutdown attempt
    throw; <-- not the current semantics
}

[bernardnormier]

As for keeping Abort... the only use case I can think of is actually for server-side connections to abort unauthorised connections and free its resources ASAP.

As we've established, Abort would not free all resources. If a middleware aborts a connection, the server would still need to DisposeAsync this connection and wait for all dispatches on this connection to complete.

Abort probably triggers a faster cleanup, but not an immediate one.

[bernardnormier]

We do indeed use a cancellation token with ConnectAsync, and count of the implementation to throw OperationCanceledException when the timeout expires.

It also seems the connection is not aborted when ConnectAsync times out. Does this make sense? I thought a connect attempt failure (including timeout) would be / should be fatal for the connection.