Use the FRAME_ENCODING_ERROR for errors in frame encoding

[marten-seemann]

Fixes #3027.

[martinthomson]

LGTM, with maybe a note on the flow control one.

[martinthomson]

This creates an extra check that some implementations might not bother to implement. Treating this as a flow control error is also fine. I note that this doesn't use a MUST either...

[marten-seemann]

Resolved by requiring one of those two.

[ianswett]

I'd be inclined towards only recommending FLOW_CONTROL_ERROR here.

[martinthomson]

I think that permitting either is in line with principles. I mean, sure, this can be flow control, but if someone has code that validates frames and that code all directs into a FRAME_ENCODING_ERROR code path, that's fine. Checking that x+y>=2^62 might be redundant, but I don't want to say that that code can't run that check.

[ianswett]

I don't think options are helpful here, so I'd prefer the more specific error.

[mikkelfj]

If there is a flow control limit < 2^62 then presumably you would check against that and not a theoretical max limits. Thus having two different errors would needlessly add an extra check.

[kazuho]

Note that there is a similar issue with MAX_STREAMS and STREAMS_BLOCKED. They communicate the cumulative count of streams of particular type using varint. It is possible to express numbers up to 2⁶²-1 however it is always an error to indicate a value above 2⁶⁰.

Section 4.5 currently states that STREAM_LIMIT_ERROR is to be used for communicating such error (for MAX_STREAMS. There is no mention of STREAMS_BLOCKED).

[martinthomson]

That was done before this proposed principle. Do you think that we should be aiming to have more specific error codes where possible here?

[kazuho]

My point is that if we are going to permit the use of FRAME_ENCODING_ERROR when the STREAM frame exceeds the 2⁶² boundary, then we should also permit the use of FRAME_ENCODING_ERROR for MAX_STREAMS frame carrying a count exceeding 2⁶⁰.

My weak preference is to not do either of the two, as we'd be losing a clear signal. IMO, this discussion is about the error code to be used when integrity check for certain frames fail. We can argue that integrity check is not part of frame decoding. Then, it would be natural to use the more specific error codes.

[janaiyengar]

I agree with @ianswett that I would prefer FLOW_CONTROL_ERROR since it is a bit more specific. Otherwise looks fine to me.

[martinthomson]

Editors discussed this and could do either. This is likely a nuisance topic for a meeting.

Editors' preference is for @marten-seemann to decide this. @marten-seemann, can you either back out the "or" on the flow control thing, or add an "or" to the stream limit frame descriptions?

[kazuho]

FWIW, I think that the "or" needs to cover the following four frames: CRYPTO, STREAMS, MAX_STREAMS, STREAMS_BLOCKED.

[martinthomson]

@marten-seemann, did you want to fix the examples listed in #3042 (comment) or shall I?

[marten-seemann]

@martinthomson, @kazuho: I added text for CRYPTO, MAX_STREAMS and STREAMS_BLOCKED frames, however:

• for CRYPTO frames, there's no flow control. It would be awkward to use a FLOW_CONTROL_ERROR here, so I opted for just using a FRAME_ENCODING_ERROR,
• for MAX_STREAMS frames FRAME_ENCODING_ERROR is the only applicable error. We do have a STREAM_LIMIT_ERROR, but this error is described as "Too many streams opened", and MAX_STREAMS doesn't open any streams by itself.

[martinthomson]

Those seem like reasonable conclusions to me. I'm happy taking this. Now I just need to ensure that the changelog is updated.

[kazuho]

@marten-seemann

for CRYPTO frames, there's no flow control. It would be awkward to use a FLOW_CONTROL_ERROR here, so I opted for just using a FRAME_ENCODING_ERROR,

I'm not sure if I follow the logic here.

If there is no flow control, then I'd presume that exceeding 2⁶² bytes is totally fine, and therefore that that the receiver should not be required (i.e. MUST) to send an error.

[marten-seemann]

@kazuho There's no flow control in the sense that you don't advertise a window and you don't update any window.

[kazuho]

@marten-seemann Are you suggesting that the CRYPTO stream has implicit limit of 2⁶² bytes that can be transferred? I think my question is how you would justify it. My claim is that it's an unnecessary complexity.

[marten-seemann]

@kazuho That's what's written in the spec. I was not happy with our decision from Tokio to not communicate a limit for the data that can be sent on a CRYPTO stream, and have peers just drop any frames sent on that stream in the future instead. But that's what the working group decided to do.

[kazuho]

@marten-seemann I think you are conflating two different issues. #2524 is an issue about the amount of data buffered, while this issue is about the maximum offset of data an endpoint can ultimately represent.

Let me more practically explain why "MUST respond with a FRAME_ENCODING_ERROR" for a CRYPTO stream is a problem.

The reason we have allowed the use of FLOW_CONTROL_ERROR for STREAM frames is because that is an error that can be caught by the stream-level receive buffer (and it's logic), rather than the frame decoder.

Stating that the error code MUST be FRAME_ENCODING_ERROR for CRYPTO streams in this particular case goes against that agreement.

That logic that we used for STREAM frames can be equally applied to CRYPTO streams. The error code can either be CRYPTO_BUFFER_EXCEEDED or FLOW_CONTROL_ERROR. I don't mind either. But requiring FRAME_ENCODING_ERROR is different, because it could require the stream-level logic to respond either with a stream-level error code (CRYPTO_BUFFER_EXCEEDED or FLOW_CONTROL_ERROR) and frame-level error code (FRAME_DECODING_ERROR) based on the input. As can be seen, this requirement to distinguish between the two cases is the same as the one we argue for the STREAM frame case.

[marten-seemann]

@kazuho You're right. I missed that we have a CRYPTO_BUFFER_EXCEEDED error. I'll prepare a follow-up PR later today. Unfortunately, it looks like it won't make it into -24 any more though.