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

Error codes #211

Closed
martinthomson opened this issue Jan 24, 2017 · 12 comments
Closed

Error codes #211

martinthomson opened this issue Jan 24, 2017 · 12 comments
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@martinthomson
Copy link
Member

There are LOTS of error codes. The only sensible thing to do is use those code to steer the receiver in the direction of the right logic for recovering from the error. There is a different piece of information that might be provided to an application for diagnostic purposes (the reason phrase helps if it goes across the wire) and there might be other needs for an API, but across the wire we can probably cut this down a lot.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Jan 24, 2017
@martinthomson
Copy link
Member Author

Note that #374 fixes this for TLS.

@mnot mnot added this to Odd Jobs in QUIC Apr 28, 2017
@mnot mnot changed the title Narrow the error code space Error codes Jun 21, 2017
@mnot mnot moved this from Odd Jobs to Middleboxen in QUIC Jun 21, 2017
@mnot mnot moved this from Middleboxen to Closing, Shutdown, Reset in QUIC Jun 21, 2017
@martinthomson
Copy link
Member Author

Moving discussion from #467 here.

@janaiyengar proposed retaining the following error codes. Here are my reasons for removing them:

If the intent is to signal to the other side, then I can't see how HANDSHAKE_TIMEOUT or NETWORK_IDLE_TIMEOUT would be sent, so I removed it.

Similar to the timeouts, UNENCRYPTED_STREAM_DATA, INVALID_VERSION_NEGOTIATION_PACKET, and ADDRESS_VALIDATION_FAILURE can't be sent in anything. Maybe if we managed to come to some conclusion about how to signal handshake errors (#608), then maybe this sort of error can be restored.

I have no idea how to signal PUBLIC_RESET_RECEIVED either.

TOO_MANY_RTOS seems to fall into the same category as the timeouts, but I can't tell because it isn't referenced anywhere.

All of the above seem to be best suited to passing across the interface between a QUIC stack and the application that uses it.

I removed CRYPTO_STREAM_CLOSED and PAYLOAD_MISSING on the basis that these were too specific, but I'm happy to retain them. I'd prefer to open issues tracking their restoration so that we can add text on when they are generated (see below).

Finally, none of these error codes was referenced by text in the document. If the document doesn't describe the conditions for generating the code, then they clearly weren't that important.

@ianswett
Copy link
Contributor

Thanks for enumerating the removed error codes.

I suspect we'll want to reinstate some of those, but none of them seem pressing at the moment.

To clarify, the replacement for HANDSHAKE_TIMEOUT and NETWORK_IDLE_TIMEOUT is NO_ERROR?

@MikeBishop
Copy link
Contributor

  • CRYPTO_STREAM_CLOSED is duplicative of QUIC_CLOSED_CRITICAL_STREAM, and when you generate it is already covered in the transport doc.
  • UNENCRYPTED_STREAM_DATA might be sent in something, depending when we receive it. This actually seems like a worthwhile one to get out: "You sent stream X != 0 in an unencrypted packet."

@martinthomson
Copy link
Member Author

QUIC_CLOSED_CRITICAL_STREAM really only applies to stream 0, doesn't it? I mean, if you take the view (as I do) that the transport shouldn't be closing streams at all (#485, @mnot keeps renaming my issues, which is really annoying), then the transport can't know about the special status of any other streams.

After the handshake, UNENCRYPTED_STREAM_DATA means packets that would be discarded. If you do anything else, you open yourself up to unauthenticated packets closing your connection. I don't know how to send this error code.

@mnot
Copy link
Member

mnot commented Jun 28, 2017

I will continue to annoy people who open needlessly specific issues; it'd be nice to stall issue #1000 until at least early next year. I'll also annoy those who make the issue title over-long, so that we can't fit all of the issue descriptions in a reasonable amount of space.

@MikeBishop
Copy link
Contributor

@martinthomson, the application can also use these error codes, or so I've been assuming. So the HTTP layer will generate QUIC_CLOSED_CRITICAL_STREAM if the session control stream is closed. You could split that up, but then every application will wind up defining its own version of "you broke it, dummy!".

True, after the handshake. I envision this as being the following scenario:

  • Application has called the QUIC library to open a connection and dumped data into the first streams in hopes of 0-RTT (or 0.5 RTT on the server)
  • QUIC library accidentally leaks this pending data into handshake cleartext packets
  • Recipient generates this error on the affected streams or connection, depending on how aggressive it wants to be.

It's a fairly specific bug, sure. It's effectively not possible after the handshake. If you want to generalize it somehow, go for it. But it is a valid form of peer misbehavior that you might want to signal to the other side.

@martinthomson
Copy link
Member Author

I'm increasingly of the opinion that applications shouldn't be using QUIC error codes, but you are right that doing that means some duplication of work across applications. Based on what we currently have, the generic codes are either properly generic (NO_ERROR, INTERNAL_ERROR) or they are maybe only applicable to a subset of applications (CLOSED_CRITICAL_STREAM, CANCELLED). I can't properly justify generalizing those codes at this point. Management of error code spaces is not a feature that the transport needs to do, and meddling in that space is where we are currently headed. I'd prefer to leave the RST_STREAM error code open for use by applications and leave the semantics of the code points entirely unspecified in QUIC.

@mnot
Copy link
Member

mnot commented Jun 28, 2017

I'm increasingly of the opinion that applications shouldn't be using QUIC error codes

@martinthomson does that deserve to be a seperate (arch) issue?

@MikeBishop
Copy link
Contributor

MikeBishop commented Jun 28, 2017 via email

@martinthomson
Copy link
Member Author

I should have been clearer. I think that we definitely need to have a way for the application to kill the connection. That suggests that the error code in the transport could carry an application error code, in much the same way as we carry a frame number, I guess. But that's details we can work out. I want to know if there is enough interest in this for me to work up a proposal.

@mcmanus
Copy link
Contributor

mcmanus commented Jun 28, 2017 via email

@martinthomson martinthomson removed this from Connection End in QUIC Oct 19, 2017
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

5 participants