-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Get tests running for HTTP/3 #31898
Get tests running for HTTP/3 #31898
Conversation
@@ -220,7 +220,7 @@ private void OnByte(byte b, IHttpHeadersHandler handler) | |||
} | |||
break; | |||
case State.CompressedHeaders: | |||
switch (BitOperations.LeadingZeroCount(b)) | |||
switch (BitOperations.LeadingZeroCount(b) - 24) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely add a comment for this.
@@ -69,6 +71,40 @@ static Certificates() | |||
public static X509Certificate2 GetNoEKUCertificate() => new X509Certificate2(s_noEKUCertificate); | |||
public static X509Certificate2 GetSelfSignedServerCertificate() => new X509Certificate2(s_selfSignedServerCertificate); | |||
public static X509Certificate2 GetSelfSignedClientCertificate() => new X509Certificate2(s_selfSignedClientCertificate); | |||
|
|||
public static X509Certificate2 GetTls13ServerCertificate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo, I should try using this in Kestrel as well 😄
} | ||
|
||
// 0-byte write to force QUIC to allocate a stream ID. | ||
await quicStream.WriteAsync(Array.Empty<byte>(), cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here a few thoughts I have:
- What if we make quicStream.StreamId nullable and return null if the stream hasn't started yet?
- Why do you need to set the Http3RequestStream.StreamId here? Can you just always call into quicStream.StreamId to get the streamId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, to track goaway, I think we may need to move this to after first write rather than here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we make quicStream.StreamId nullable and return null if the stream hasn't started yet?
Having StreamId
populated late is an unfortunate usability issue; I'd rather it just be usable immediately after construction if it's reasonable to do so. I think we need to better understand this from msquic perspective; my feeling is that it is behaving like this (presumably) as a corner-case optimization to avoid allocating unused stream IDs, as QUIC "allocates" gaps in stream IDs it sees and counts them against the maximum. @nibanks do you have any background here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you start it in your constructor then? You don't need to delay starting only when you need to send on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I remember incorrectly, you said that starting a stream really should be async.
Before the first write or send, there is no async operations in between. We could add a StartAsync method to the stream that's called immediately after creating the stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General thoughts, but I don't think there is any reason to block this.
a232aa6
to
9a8a941
Compare
@@ -224,7 +224,7 @@ private void OnByte(byte b, IHttpHeadersHandler handler) | |||
} | |||
break; | |||
case State.CompressedHeaders: | |||
switch (BitOperations.LeadingZeroCount(b) - 24) | |||
switch (BitOperations.LeadingZeroCount(b) - 24) // byte 'b' is extended to uint, so will have 24 extra 0s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (BitOperations.LeadingZeroCount(b) - 24) // byte 'b' is extended to uint, so will have 24 extra 0s. | |
switch (BitOperations.LeadingZeroCount(b) - sizeof(uint) + sizeof(byte)) // byte 'b' is extended to uint, so will have 24 extra 0s. |
…Http20, and update some HTTP/2 to work for HTTP/3. Enable tests for HTTP/3, behind a conditional feature test. Fix QPackDecoder lzcnt assuming an 8-bit test. Rename test QPACK classes from QPackEncoder/QPackDecoder -> QPackTestEncoder/QPackTestDecoder to avoid naming confusion with product code classes. Fix QPackTestDecoder bit flag checks. Fix a double call to QuicConnection.CloseAsync(). Update to shutdown QuicConnection in a background task. Fix test cert usage.
4708ff4
to
a7dca57
Compare
This PR gets a minimal set of tests working, but not all of them. It enables all HTTP/3 tests, even the not yet working ones, if you have MsQuic support. Our CI does not have MsQuic support, so the failing tests will not cause CI failures.