Skip to content
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

Abort pending tasks with ObjectDisposedException #4045

Merged
merged 30 commits into from
Sep 12, 2024
Merged

Conversation

pepone
Copy link
Member

@pepone pepone commented Sep 10, 2024

Fix #3990

.NET 9 updated QUIC implementation to cancel pending task with ODE, when the listener or connection are disposed. This PR updates the failing tests and align Slic with the new QUIC behavior.

@@ -32,13 +35,20 @@ internal sealed class TcpListener : IListener<IDuplexConnection>
}
catch (SocketException exception)
{
if (exception.SocketErrorCode == SocketError.OperationAborted)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated Tcp/Coloc listeners to do the same. Alternatively we can move this to Slic and don't update Tcp/Coloc behavior.

@@ -21,7 +21,7 @@ public async Task Ssl_client_connection_connect_fails_when_server_provides_untru
.AddSingleton(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes required to build conformance tests with .NET9

[Test]
[Ignore("TODO: Fix https://github.com/icerpc/icerpc-csharp/issues/3990")]
public async Task Connection_dispose_aborts_pending_operations_with_operation_aborted_error()
public async Task Connection_dispose_aborts_pending_operations_with_object_disposed_exception()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just test the new behavior with .NET 9 and greater.

Copy link
Member

@bernardnormier bernardnormier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised there is no change to the Ice/IceRpc ProtocolConnection code that calls these transport APIs.

Even if this code already handles ObjectDisposedException / IceRpcError.OperationAborted the same way, it would make sense to remove the OperationAborted handling from IceProtocolConnection (since no QUIC/Slic for the ice protocol) and add a comment to the IceRpcProtocolConnection code (keep OperationAborted for .NET 8 QUIC support?).

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@pepone
Copy link
Member Author

pepone commented Sep 12, 2024

I am surprised there is no change to the Ice/IceRpc ProtocolConnection code that calls these transport APIs.

I have updated the server code to expect an ObjectDisposedException instead of an IceRpcException(OperationAborted) when calling the AcceptAsync listener operation.

Regarding the IceRpcProtocolConnection, it's unaffected by the changes to CreateStreamAsync and AcceptStreamAsync. In this case, when the connection is disposed, it cancels the cancellation token passed to these operations before disposing of the transport connection. As a result, both CreateStreamAsync and AcceptStreamAsync will still fail with the expected OperationCanceledException. Therefore, the code doesn't need to handle neither IceRpcException(OperationAborted) nor ObjectDisposedException.

Lastly, the IceProtocolConnection does not use any of the updated APIs.

@bernardnormier
Copy link
Member

Regarding the IceRpcProtocolConnection, it's unaffected by the changes to CreateStreamAsync and AcceptStreamAsync. In this case, when the connection is disposed, it cancels the cancellation token passed to these operations before disposing of the transport connection

Do you mean it cancels the CTS they use and then waits for these tasks to complete before disposing the connection?

// The AcceptAsync call can fail with OperationAborted during shutdown if it is accepting a connection while the
// listener is disposed.
(exception is IceRpcException rpcException && rpcException.IceRpcError != IceRpcError.OperationAborted) ||
(exception is IceRpcException rpcException) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify - maybe exception is IceRpcException or AuthenticationException?

catch (ObjectDisposedException)
{
cancellationToken.ThrowIfCancellationRequested();
throw;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This additional fix is to ensure QUIC and Slic behave the same. CreateStreamAsync throw OperationCanceledException if ODE is received and task is already canceled.

@pepone
Copy link
Member Author

pepone commented Sep 12, 2024

Do you mean it cancels the CTS they use and then waits for these tasks to complete before disposing the connection?

I think there was a difference here with Slic and QUIC in CreateStreamAsync.

If you cancel the cancellation token passed to CreateStreamAsync and then dispose the connection.

Slic throws OperationCancelledException, Quic throws ObjectDisposedException, I fixed Quic connection to behave as Slic adding:

        catch (ObjectDisposedException)
        {
            cancellationToken.ThrowIfCancellationRequested();
            throw;
        }

@pepone pepone merged commit 72ffe5d into icerpc:main Sep 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QUIC conformance test failure with .NET 9.0
3 participants