Give codepoints UPPER_CASE names #546
Conversation
|
I like this change and upper case naming convention. |
| as a 'Protocol Violation'. Objects MUST NOT be sent on the control stream, and a | ||
| peer receiving an Object on the control stream closes the session as a 'Protocol | ||
| Violation'. | ||
| as a SESSION_PROTOCOL_VIOLATION. Objects MUST NOT be sent on the control stream, |
There was a problem hiding this comment.
nit: Isn't it a bit redundant to close the session with a SESSION_PROTOCOL_VIOLATION? I think PROTOCOL_VIOLATION might be sufficient?
Similar comment for the other error codes, where I don't think SESSION_ is necessary
There was a problem hiding this comment.
Prior to this change, the document defines three error code registries, leading to three INTERNAL_ERRROR definitions, using two different values. That's ambiguous and annoying.
I agree the naming with a prefix is ugly but I wanted some some way to resolve the ambiguity. Happy to consider alternatives though. One approach would be to have just a single unified error code space for MoQ.
There was a problem hiding this comment.
Isn't it that there is only one/single bidirectional stream used solely for control messages and unidirectional streams only for data objects? Why would the implementation even process a control message over a unidirectional stream? IMO, this should be reworded a bit....
It's a DATA_STREAM_VIOLATION if any other message type is received (regardless of control or other stream header/data object header) over a unidirectional stream that isn't expected based on the stream mode in progress.
If a new bidir stream is created or if data objects/stream headers are received over the control bidir stream, it would make sense to be a protocol violation that would result in session/connection close.
My two-cents are that if a data (aka unidirectional) stream violates something, such as a bad/malformed message header (e.g., believed to be control message type erroneously), it shouldn't result in a connection/session close. It should only result in the unidirectional stream being closed. A use-case for this is a forced reset of a stream to transition to a new stream.
There was a problem hiding this comment.
I'd rather track adjusting the "when" of errors in a separate issue. This PR is already going to be hard to review, even when it focuses on the "what" of the error codes names/values.
| * GOAWAY Timeout: The session was closed because the client took too long to | ||
| close the session in response to a GOAWAY ({{message-goaway}}) message. | ||
| See session migration ({{session-migration}}). | ||
| SESSION_NO_ERROR (0x0): |
There was a problem hiding this comment.
When using native QUIC, are we currently conflicting with QUIC error codes?
There was a problem hiding this comment.
the text above says
When native QUIC is used, the session is closed using the CONNECTION_CLOSE frame ({{QUIC, Section 19.19}}).
It should probably be clarified that this is an application-layer close, and that MoQT over native QUIC is an application mapping that defines its own error code registry (like HTTP/3 does in https://datatracker.ietf.org/doc/html/rfc9114#name-http-3-error-codes)
In other words, the QUIC errors are in the "transport namespace" and so there is no conflict.
There was a problem hiding this comment.
+1. We currently map these values as the QUIC application error code.
| |------|---------------------------| | ||
| | 0x5 | Timeout | | ||
| |------|---------------------------| | ||
| SUBSCRIBE_ERROR_INTERNAL_ERROR (0x0): |
There was a problem hiding this comment.
It'd be nice to not have the 'ERROR_' in the names, but I see you're trying to distinguish them from 'STATUS_'. The fact we have two sets of error codes may be an error, which this PR is pointing out?
There was a problem hiding this comment.
agree with @ianswett . Looks like these needs to be collapsed
There was a problem hiding this comment.
Totally agree. I was very confused by these two different similar but different error code sets. On https://github.com/moq-wg/moq-transport/pull/546/files#r1773547847 I also highlight that differentiating SESSION and SUBSCRIBE is annoying. Maybe we just want a unified space?
There was a problem hiding this comment.
I don't have a strong opinion, though a downside of a unified space is that you can receive an error code that has no meaning in a particular context.
There was a problem hiding this comment.
Yes that's true. But the mantra in QuIc and H3 has been that you can't always expect the peer to be able to (or want to) use the error code that the RFC states.
I'm not familiar enough with MoQ to know what it's mantra wants to be
|
25 Sep 2024 Interim: do another pass consolidating into a single error space, then @ianswett merge after discussion wanes. |
ianswett
added
the
Editorial
Not 100% sure if we want to go the whole hog, but this addresses the issue I raise on #545.
Importantly, there's a few things that have naming clashes, which I solved by adding a long prefix. Something to bash on.