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

Add new KeepAliveInterval option in QuicClientTransportOptions #3991

Merged
merged 5 commits into from
May 23, 2024

Conversation

bernardnormier
Copy link
Member

This PR adds the new KeepAliveInterval option to QuicClientTransportOptions. It's implemented only with .NET 9.0.

Fixes #3353.

@bernardnormier bernardnormier requested review from pepone and bentoi May 22, 2024 14:44
/// provided by the underlying implementation".</value>
/// <remarks>The idle timeout is negotiated by QUIC during connection establishment. The actual idle timeout value
/// for a connection can be lower than the value you've set here.</remarks>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.FromSeconds(30);
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the default from Zero to 30 seconds - which is the msquic default (= what Zero meant). It's clearer to spell out the actual default, and it's highly desirable to use the same default in all IceRPC implementation.

@@ -43,7 +43,7 @@ public class CompressorInterceptorTests
new BrotliStream(outStream, CompressionMode.Decompress) :
new DeflateStream(outStream, CompressionMode.Decompress);
var decompressedPayload = new byte[4096];
await decompressedStream.ReadAsync(decompressedPayload);
await decompressedStream.ReadAtLeastAsync(decompressedPayload, _payload.Length);
Copy link
Member Author

Choose a reason for hiding this comment

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

These are tiny unrelated fixes for analyzer warnings introduced in .NET 9.0.

// Without the keep-alive interval, the idle timer aborts the connection after 500ms.
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2));

#if NET9_0_OR_GREATER
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 assumes the unit tests are built with the same version of .NET as the IceRpc.Transport.Quic assembly (a safe assumption).

@bernardnormier bernardnormier requested a review from externl May 22, 2024 14:48
// Act/Assert

// Without the keep-alive interval, the idle timer aborts the connection after 500ms.
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2));
Copy link
Member

Choose a reason for hiding this comment

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

I think would be clear to write the response and close the stream after the delay.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I reworked the test and removed the cancellation token source.

Note that with QUIC, canceling a read is fatal to the stream. You can't cancel reading and then start reading again. We may be doing the same in Slic.

@bernardnormier bernardnormier requested a review from pepone May 22, 2024 18:37
@bernardnormier bernardnormier merged commit 7f9f6ab into icerpc:main May 23, 2024
3 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.

Missing Quic keep alive option to enable keep alive
3 participants