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

HTTP/3 fixed some tests #51493

Merged
merged 3 commits into from
Apr 22, 2021
Merged

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Apr 19, 2021

Some HTTP/3 tests fail when ran with msquic. This is an attempt to fix some of them.
Also removed H/3 from versioning tests since we cannot do upgrades/downgrades with HTTP/3 over the same connection (TCP vs UDP). And we already have tests for Alt-Svc upgrade.

@ghost
Copy link

ghost commented Apr 19, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Some HTTP/3 tests fail when ran with msquic. This is an attempt to fix some of them.

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@@ -24,7 +24,7 @@ public Http3LoopbackServer(QuicImplementationProvider quicImplementationProvider
{
options ??= new GenericLoopbackOptions();

_cert = Configuration.Certificates.GetSelfSigned13ServerCertificate();
_cert = Configuration.Certificates.GetServerCertificate();
Copy link
Member Author

Choose a reason for hiding this comment

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

@wfurt I don't know if this change is what we want/need. but it did help with some of the failing tests.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it really matters. CI may be set up to trust your contoso chain but not self-signed. Either one should be fine once we have custom validation in place.

@ManickaP ManickaP marked this pull request as ready for review April 19, 2021 14:34
Debug.Assert(!_state.ShutdownTcs.Task.IsCompleted);
if (_state.ShutdownTcs.Task.IsCompleted)
{
return ValueTask.CompletedTask;
Copy link
Member

Choose a reason for hiding this comment

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

If we want to have a fair no-op, we should check that flags are the same as they were the last time... or at least check IsCompletedSuccessfully instead of IsCompleted - otherwise we would not be able to send an abortive shutdown after we timeouted waiting for graceful shutdown to complete (though, I'm not 100% sure msquic supports it 😬)

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, this is very shortsighted change. I'll just leave out the assert and handle the multiple calls to ShutdownAsync on the Http3Connection side.

Since H3 is using QUIC/UDP it's not possible to use Ssl over TCP socket ALPN to determine the final request version. It either has to be pre-negotiated or upgraded via Alt-Svc.
@ManickaP ManickaP force-pushed the mapichov/msquic_h3_test_errors branch from 2bf3b20 to eef5344 Compare April 19, 2021 18:40
@@ -481,6 +481,10 @@ private async Task AcceptStreamsAsync()
_ = ProcessServerStreamAsync(stream);
}
}
catch (QuicOperationAbortedException)
{
// Shutdown initiated by us, no need to abort.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to make a second Abort a no-op? I see the same pattern down below in ProcessServerStreamAsync -- is possibly exists in other places as well:

try
{ ... }
catch (Exception ex)
{
    Abort(ex);
}

so it may be better to treat it inside Abort?

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 doesn't come from an abort though, this is a reaction to a normal shutdown initiated by us. The QuicOperationAbortedException here is thrown by the connection as reaction to AcceptQueue being closed:

try
{
stream = await _state.AcceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false);
}
catch (ChannelClosedException)
{
throw _state.AbortErrorCode switch
{
-1 => new QuicOperationAbortedException(), // Shutdown initiated by us.
long err => new QuicConnectionAbortedException(err) // Shutdown initiated by peer.
};
}

The code you're referencing is handling QuicOperationAbortedException caused by a different action:

else if (_state.ReadState == ReadState.Aborted)
{
throw _state.ReadErrorCode switch
{
-1 => new QuicOperationAbortedException(),
long err => new QuicStreamAbortedException(err)
};

So, I don't think combining them is a correct solution here.

I do concur that the utilization of QuicOperationAbortedException to notify the user that the connection won't provide any more stream from AcceptStreamAsync might be questionable here. But that's not a goal of this PR, I'm just trying to make it work.

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Would still wait for @wfurt reply on certs though...

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@ManickaP ManickaP closed this Apr 22, 2021
@ManickaP ManickaP reopened this Apr 22, 2021
@ManickaP ManickaP merged commit ab4e593 into dotnet:main Apr 22, 2021
@ManickaP ManickaP deleted the mapichov/msquic_h3_test_errors branch April 27, 2021 11:54
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants