h3spec

[Ruben2424]

Hi,
i solved #110.
Some tests are failing. I will see if i can fix those.

[seanmonstar]

You're amazing!

[camshaft]

I recommend linking to the page on rfc editor so it's easier to click the link and read the section

Suggested change

	//= ci/compliance/specs/rfc9114.txt#7.2.4.1
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.4.1

[eagr]

My bad. Will fix this real quick.

[Ruben2424]

My bad. Will fix this real quick.

Ok thanks.

[camshaft]

I would consider quoting the whole paragraph to give context:

Suggested change

	//# their receipt MUST be treated as a connection error of type
	//# H3_SETTINGS_ERROR.
	//# Setting identifiers that were defined in [HTTP/2] where there is no
	//# corresponding HTTP/3 setting have also been reserved
	//# (Section 11.2.2). These reserved settings MUST NOT be sent, and
	//# their receipt MUST be treated as a connection error of type
	//# H3_SETTINGS_ERROR.

[Ruben2424]

Hi @seanmonstar ,
i fixed many of the http/3 failures, but some are left.

1. MUST send H3_MESSAGE_ERROR if a pseudo-header is duplicated [HTTP/3 4.1.1] FAILED [4]
   I didn´t found that in the rfc, but maybe i just missed it.
2. MUST send H3_FRAME_UNEXPECTED if CANCEL_PUSH is received in a request stream [HTTP/3 7.2.5] FAILED [5]
   In the accept method the poll_next returns an Error: received incomplete frame.
   Also if the frame is received after the accept method is over how to close the connection without a reference to it? Maybe with a channel to the driver?
3. MUST send QPACK_ENCODER_STREAM_ERROR if a new dynamic table capacity value exceeds the limit [QPACK 4.1.3] FAILED [6]
MUST send QPACK_DECODER_STREAM_ERROR if Insert Count Increment is 0 [QPACK 4.4.3] FAILED [7]
   These two i know not enough about qpack jet to debug why these are failing.

Also there are some Quic failures.
I skiped the 0-RTT one because the server-example crashes with this test.
What should be done with the Quic failures?

And there are many similar match expressions to handle the errors. I can think about a cleaner solution if you want. But i don´t have much time next week.
If you want to merge this this week, i can skip the failing tests, so the ci is green.

[seanmonstar]

It could be the h3spec is using requirements from an old version of the spec... In that case, is there a way to ignore some?

Another option is to make your commits that fix various cases a separate PR, and we can merge that sooner, since they're straight improvements, while we figure out how to make the rest of h3spec happy.

[camshaft]

We had to ignore a few test cases in s2n-quic, so it's doable: https://github.com/aws/s2n-quic/blob/dce3fd056f9a263d6fd97de932c9864d73a7e92f/.github/workflows/qns.yml#L556

[Ruben2424]

The failing tests now getting ignored.

The QUIC servers/MUST send PROTOCOL_VIOLATION if CRYPTO in 0-RTT is received [TLS 8.3]
lets the example server panic so it crashes. That should probably be fixed, but I didn´t found the cause of the Problem.

[seanmonstar]

I'd probably make the error level a concept of the Error, and not the Code (I'd expect the code to be mostly only a number). Though, I'm also open to hearing why it should be part of Code directly...

[Ruben2424]

I did this to be able to define a specific ErrorLevel for a specific Code.
But maybe a impl From<Code> for ErrorLevel would also do it?

Then the ErrorLevel could be part of the Application like Code and Reason.

[seanmonstar]

After having reviewed the rest, I now lean even more that Code should just be the number. I noticed all the constants in this PR needed to make them connection error level codes, whereas the spec just defines them separate. The code combined with a context turns it into a connection or stream level error.

[Ruben2424]

Hi, sorry for the late response.
I moved the ErrorLevel away from the Code struct.

[seanmonstar]

It'd be good to keep this as an "unstable" method. The Kind is not meant to be matched on outside of h3 (and the tests), since we might change it at anytime.