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
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
78ad104
Abort pending tasks with ObjectDisposedException
pepone Sep 10, 2024
a0a82cc
Merge remote-tracking branch 'origin/main' into issue#3990
pepone Sep 11, 2024
8ef7ee9
Fix review comments
pepone Sep 11, 2024
4be4dba
Run CI workflow on macOS, Windows, and Linux
externl Sep 11, 2024
ab39d88
Don't test templates on Windows
externl Sep 11, 2024
ce1fecf
Incrase timeout
externl Sep 11, 2024
cd5f306
Remove blame-hang-timeout
externl Sep 11, 2024
e82ee71
testing
externl Sep 11, 2024
f4582c9
test
externl Sep 11, 2024
60a4638
test
externl Sep 11, 2024
8acf0e5
test
externl Sep 11, 2024
f5c195f
test
externl Sep 11, 2024
d1ef7c2
diag
externl Sep 11, 2024
15418ac
fix
externl Sep 11, 2024
b774686
workers
externl Sep 11, 2024
be82e22
fix
externl Sep 11, 2024
e86e6ae
test
externl Sep 11, 2024
0f5fb98
2-workers
externl Sep 11, 2024
1603901
logger
externl Sep 11, 2024
dffbd02
test
externl Sep 11, 2024
899bc65
Upgrade handling of exception from listener
pepone Sep 12, 2024
18e0a5a
Test fixes from #4035
pepone Sep 12, 2024
336b073
Merge remote-tracking branch 'joe/ci-matrix' into issue#3990
pepone Sep 12, 2024
ec9bf04
Fix accept fatal failure test
pepone Sep 12, 2024
0b65f28
Mark keys as exportable for compatibility with macOS
pepone Sep 12, 2024
2ee43a3
Review fixes
pepone Sep 12, 2024
e9fb698
Merge remote-tracking branch 'origin/main' into issue#3990
pepone Sep 12, 2024
fa5f4b9
Revert CI updates
pepone Sep 12, 2024
586a67c
Additional fixes for IceRpc protocol connection
pepone Sep 12, 2024
389d597
QuicConnection fixes
pepone Sep 12, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion .github/actions/test/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,13 @@ runs:
run: cargo test --manifest-path tools/slicec-cs/Cargo.toml
shell: bash
- name: 🧪 Test
run: dotnet test --no-build --verbosity normal --blame-hang-timeout 8m
# See https://github.com/microsoft/vstest/issues/2080#issuecomment-539879345
run: dotnet test --no-build --verbosity normal --blame-hang-timeout 10s --logger:"console;noprogress=true"
shell: bash
- name: Upload hang dumps
uses: actions/upload-artifact@v4
with:
name: hang-dumps
path: ${{ github.workspace }}/**/*.dmp
if-no-files-found: ignore
if: failure()
14 changes: 12 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,19 @@ on:
pull_request:
branches: ["main"]

# See https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-a-fallback-value
concurrency:
group: ${{ github.head_ref || github.run_id }}
cancel-in-progress: true

jobs:
build_and_test:
runs-on: ubuntu-latest
timeout-minutes: 10
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
timeout-minutes: 20
steps:
- name: Checkout
uses: actions/checkout@v4
Expand All @@ -24,6 +33,7 @@ jobs:
shell: bash
- name: Test Templates
uses: ./.github/actions/test-templates
if: runner.os != 'Windows'
- name: Build Examples
uses: ./.github/actions/build-examples
- name: Build DocFX Examples
Expand Down
5 changes: 4 additions & 1 deletion src/IceRpc.Transports.Coloc/Internal/ColocListener.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) ZeroC, Inc.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO.Pipelines;
using System.Net;
Expand Down Expand Up @@ -56,7 +57,9 @@ internal class ColocListener : IListener<IDuplexConnection>
catch (OperationCanceledException)
{
cancellationToken.ThrowIfCancellationRequested();
throw new IceRpcException(IceRpcError.OperationAborted);
// The accept operation was canceled because the listener was disposed.
Debug.Assert(_disposeCts.IsCancellationRequested);
throw new ObjectDisposedException($"{typeof(ColocListener)}");
}
}

Expand Down
13 changes: 3 additions & 10 deletions src/IceRpc/Server.cs
Original file line number Diff line number Diff line change
Expand Up @@ -647,9 +647,7 @@ await Task.WhenAll(

/// <summary>Returns true if the <see cref="IConnectorListener.AcceptAsync" /> failure can be retried.</summary>
private static bool IsRetryableAcceptException(Exception exception) =>
// 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?

// Transports such as Quic do the SSL handshake when the connection is accepted, this can throw
// AuthenticationException if it fails.
exception is AuthenticationException;
Expand All @@ -674,12 +672,6 @@ private class LogConnectorListenerDecorator : IConnectorListener
new LogConnectorDecorator(connector, ServerAddress, remoteNetworkAddress, _logger),
remoteNetworkAddress);
}
catch (IceRpcException exception) when (exception.IceRpcError == IceRpcError.OperationAborted)
{
// Do not log this exception. The AcceptAsync call can fail with OperationAborted during shutdown if it
// is accepting a connection while the listener is disposed.
throw;
}
catch (OperationCanceledException exception) when (exception.CancellationToken == cancellationToken)
{
// Do not log this exception. The AcceptAsync call can fail with OperationCanceledException during
Expand All @@ -689,7 +681,8 @@ private class LogConnectorListenerDecorator : IConnectorListener
catch (ObjectDisposedException)
{
// Do not log this exception. The AcceptAsync call can fail with ObjectDisposedException during
// shutdown once the listener is disposed.
// shutdown once the listener is disposed or if it is accepting a connection while the listener is
// disposed.
throw;
}
catch (Exception exception) when (IsRetryableAcceptException(exception))
Expand Down
7 changes: 3 additions & 4 deletions src/IceRpc/Transports/Slic/Internal/SlicConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ public async ValueTask<IMultiplexedStream> AcceptStreamAsync(CancellationToken c
}
catch (ChannelClosedException exception)
{
ObjectDisposedException.ThrowIf(_disposeTask is not null, this);
Debug.Assert(exception.InnerException is not null);

// The exception given to ChannelWriter.Complete(Exception? exception) is the InnerException.
throw ExceptionUtil.Throw(exception.InnerException);
}
Expand Down Expand Up @@ -439,7 +439,7 @@ public async ValueTask<IMultiplexedStream> CreateStreamAsync(
catch (OperationCanceledException)
{
cancellationToken.ThrowIfCancellationRequested();

ObjectDisposedException.ThrowIf(_disposeTask is not null, this);
Debug.Assert(_isClosed);
throw new IceRpcException(_peerCloseError ?? IceRpcError.OperationAborted, _closedMessage);
}
Expand All @@ -458,8 +458,6 @@ public async ValueTask<IMultiplexedStream> CreateStreamAsync(

public ValueTask DisposeAsync()
{
TryClose(new IceRpcException(IceRpcError.OperationAborted), "The connection was disposed.");

lock (_mutex)
{
_disposeTask ??= PerformDisposeAsync();
Expand All @@ -470,6 +468,7 @@ async Task PerformDisposeAsync()
{
// Make sure we execute the code below without holding the mutex lock.
await Task.Yield();
TryClose(new IceRpcException(IceRpcError.OperationAborted), "The connection was disposed.");

_disposedCts.Cancel();

Expand Down
12 changes: 11 additions & 1 deletion src/IceRpc/Transports/Tcp/Internal/TcpListener.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ internal sealed class TcpListener : IListener<IDuplexConnection>
private readonly MemoryPool<byte> _pool;
private readonly Socket _socket;

// Set to 1 when the listener is disposed.
private volatile int _disposed;

public async Task<(IDuplexConnection, EndPoint)> AcceptAsync(CancellationToken cancellationToken)
{
try
Expand All @@ -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.

{
ObjectDisposedException.ThrowIf(_disposed == 1, this);
}
throw exception.ToIceRpcException();
}
}

public ValueTask DisposeAsync()
{
_socket.Dispose();
if (Interlocked.Exchange(ref _disposed, 1) == 0)
{
_socket.Dispose();
}
return default;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<TargetFrameworks>net8.0;net9.0</TargetFrameworks>
<IsPackable>true</IsPackable>
<!-- Missing XML comment for publicly visible type or member. -->
<NoWarn>CS1591</NoWarn>
Expand All @@ -27,6 +27,10 @@
<ProjectReference Include="../IceRpc.Tests.Common/IceRpc.Tests.Common.csproj" ExactVersion="true" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
<PackageReference Include="Microsoft.Bcl.Cryptography" Version="9.0.0-preview.*" />
</ItemGroup>

<!-- NuGet package contents-->
<ItemGroup>
<Content Include="../../LICENSE" Pack="true" PackagePath="/" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public async Task Call_accept_on_a_listener_and_then_cancel_the_cancellation_sou
}

[Test]
public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_operation_aborted_error()
public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_object_disposed_exception()
{
// Arrange
await using ServiceProvider provider = CreateServiceCollection().BuildServiceProvider(validateScopes: true);
Expand All @@ -92,11 +92,7 @@ public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_opera
await listener.DisposeAsync();

// Assert
IceRpcException? exception = Assert.ThrowsAsync<IceRpcException>(async () => await acceptTask);
Assert.That(
exception!.IceRpcError,
Is.EqualTo(IceRpcError.OperationAborted),
$"The test failed with an unexpected IceRpcError {exception}");
Assert.That(async () => await acceptTask, Throws.TypeOf<ObjectDisposedException>());
}

[Test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ 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

new SslServerAuthenticationOptions
{
ServerCertificate = new X509Certificate2("server-untrusted.p12"),
ServerCertificate = X509CertificateLoader.LoadPkcs12FromFile(
"server-untrusted.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable),
})
.AddSingleton(
new SslClientAuthenticationOptions
Expand Down Expand Up @@ -65,15 +68,21 @@ public async Task Ssl_server_connection_connect_fails_when_client_provides_untru
{
ClientCertificateRequired = true,
RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => false,
ServerCertificate = new X509Certificate2("server.p12"),
ServerCertificate = X509CertificateLoader.LoadPkcs12FromFile(
"server.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable),
})
.AddSingleton(
new SslClientAuthenticationOptions
{
ClientCertificates = new X509CertificateCollection()
{
new X509Certificate2("client-untrusted.p12")
},
ClientCertificates =
[
X509CertificateLoader.LoadPkcs12FromFile(
"client-untrusted.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable)
],
#pragma warning disable CA5359 // Do Not Disable Certificate Validation, certificate validation is not required for these tests.
RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true
#pragma warning restore CA5359 // Do Not Disable Certificate Validation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,9 +426,11 @@ public async Task Connection_operations_fail_with_connection_aborted_error_after
$"The test failed with an unexpected IceRpcError {exception}");
}

#if NET9_0_OR_GREATER
// The QuicConnection behavior changed in .NET 9
// see: https://github.com/dotnet/runtime/pull/92215
[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.

{
// Arrange
IServiceCollection serviceCollection = CreateServiceCollection();
Expand All @@ -448,24 +450,16 @@ public async Task Connection_dispose_aborts_pending_operations_with_operation_ab

// Assert

IceRpcException? exception = Assert.ThrowsAsync<IceRpcException>(async () => await acceptStreamTask);
Assert.That(
exception?.IceRpcError,
Is.EqualTo(IceRpcError.OperationAborted),
$"The test failed with an unexpected IceRpcError {exception}");

exception = Assert.ThrowsAsync<IceRpcException>(async () => await createStreamTask);
Assert.That(
exception?.IceRpcError,
Is.EqualTo(IceRpcError.OperationAborted),
$"The test failed with an unexpected IceRpcError {exception}");
Assert.That(async () => await acceptStreamTask, Throws.TypeOf<ObjectDisposedException>());
Assert.That(async () => await createStreamTask, Throws.TypeOf<ObjectDisposedException>());

exception = Assert.ThrowsAsync<IceRpcException>(async () => await readTask);
var exception = Assert.ThrowsAsync<IceRpcException>(async () => await readTask);
Assert.That(
exception?.IceRpcError,
Is.EqualTo(IceRpcError.OperationAborted),
$"The test failed with an unexpected IceRpcError {exception}");
}
#endif

[Test]
public async Task Disposing_the_client_connection_aborts_the_server_connection()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,11 @@ public async Task Call_accept_on_a_listener_and_then_cancel_the_cancellation_sou
Assert.That(async () => await acceptTask, Throws.TypeOf<OperationCanceledException>());
}

#if NET9_0_OR_GREATER
// The QuicListener behavior changed in .NET 9
// see: https://github.com/dotnet/runtime/pull/92215
[Test]
[Ignore("TODO: Fix https://github.com/icerpc/icerpc-csharp/issues/3990")]
public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_operation_aborted_error()
public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_object_disposed_exception()
{
// Arrange
await using ServiceProvider provider = CreateServiceCollection().BuildServiceProvider(validateScopes: true);
Expand All @@ -125,12 +127,9 @@ public async Task Call_accept_on_a_listener_and_then_dispose_it_fails_with_opera
await listener.DisposeAsync();

// Assert
IceRpcException? exception = Assert.ThrowsAsync<IceRpcException>(async () => await acceptTask);
Assert.That(
exception!.IceRpcError,
Is.EqualTo(IceRpcError.OperationAborted),
$"The test failed with an unexpected IceRpcError {exception}");
Assert.That(async () => await acceptTask, Throws.TypeOf<ObjectDisposedException>());
}
#endif

[Test]
public async Task Call_accept_on_a_listener_with_a_canceled_cancellation_token_fails_with_operation_canceled_exception()
Expand Down
5 changes: 4 additions & 1 deletion tests/IceRpc.Tests/IceRpc.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<PropertyGroup>
<!-- Missing XML comment for publicly visible type or member. -->
<NoWarn>CS1591</NoWarn>
<TargetFramework>net8.0</TargetFramework>
<TargetFrameworks>net8.0;net9.0</TargetFrameworks>
</PropertyGroup>

<ItemGroup>
Expand All @@ -24,6 +24,9 @@
<ProjectReference Include="../IceRpc.Conformance.Tests/IceRpc.Conformance.Tests.csproj" />
<ProjectReference Include="../IceRpc.Tests.Common/IceRpc.Tests.Common.csproj" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
<PackageReference Include="Microsoft.Bcl.Cryptography" Version="9.0.0-preview.*" />
</ItemGroup>
<ItemGroup>
<None Include="../../certs/*.p12">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Expand Down
2 changes: 1 addition & 1 deletion tests/IceRpc.Tests/ServerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public class ServerTests

public static IEnumerable<Exception> AcceptFatalException { get; } = new Exception[]
{
new IceRpcException(IceRpcError.OperationAborted),
new ObjectDisposedException(nameof(ServerTests)),
new Exception(),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ public async Task Slic_over_ssl_client_connection_connect_fails_when_server_prov
.AddSingleton(
new SslServerAuthenticationOptions
{
ServerCertificate = new X509Certificate2("server-untrusted.p12"),
ServerCertificate = X509CertificateLoader.LoadPkcs12FromFile(
"server-untrusted.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable),
})
.AddSingleton(
new SslClientAuthenticationOptions
Expand Down Expand Up @@ -71,15 +74,21 @@ public async Task Slic_over_ssl_server_connection_connect_fails_when_client_prov
{
ClientCertificateRequired = true,
RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => false,
ServerCertificate = new X509Certificate2("server.p12"),
ServerCertificate = X509CertificateLoader.LoadPkcs12FromFile(
"server.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable),
})
.AddSingleton(
new SslClientAuthenticationOptions
{
ClientCertificates = new X509CertificateCollection()
{
new X509Certificate2("client-untrusted.p12")
},
ClientCertificates =
[
X509CertificateLoader.LoadPkcs12FromFile(
"client-untrusted.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable)
],
RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true
})
.BuildServiceProvider(validateScopes: true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ internal static IServiceCollection AddSslTest(this IServiceCollection services,
})
.AddSingleton(provider => new SslServerAuthenticationOptions
{
ServerCertificate = new X509Certificate2("server.p12")
ServerCertificate = X509CertificateLoader.LoadPkcs12FromFile(
"server.p12",
password: null,
keyStorageFlags: X509KeyStorageFlags.Exportable)
});
}
Loading
Loading