-
Notifications
You must be signed in to change notification settings - Fork 203
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
Merge CONNECTION_CLOSE and APPLICATION_CLOSE #978
Comments
I think it makes the text a bit easier to read to only have one close frame. When I first re-read the draft with them split out I was initially very confused. |
Probably true. The split leads to interesting corner cases. Also, the application close reason codes are supposedly "defined by the application". But if that's really the case, maybe they should be conveyed by an application level message. |
@larseggert, you say that they can be differentiated by the error code spaces, but I don't see that in the text. Transport error codes are a sixteen-bit space; application error codes are a different sixteen-bit space. In RST_STREAM, you know a priori that it's an application error code. In *_CLOSE, you know which is which only by the frame type. If you merged the frame types into one, you'd need a flag in the frame header, which is wire-identical to what we currently have: frame type 0b0000001A, where A is a flag indicating whether the error is transport-based (A=0) or application-based (A=1). If you think recasting it as one frame type with a flag is easier for implementers to understand, that seems purely editorial. @huitema, if applications are responsible for communicating their own reasons for fatally closing the connection, then they have to define a way to semi-reliably communicate that reason after reaching an invalid state. That's something we've already gone to some trouble to support in the transport, and it seems an unfortunate thing for the application protocol to be required to reinvent each time, particularly protocols which don't have a dedicated control stream. |
Oh. I had assumed they were sharing the same 16bit space (since that seems plenty large.) |
Managing separate ID spaces is easier (since N_applications might be large). On @huitema's suggestion, if you think about that, it's hard. The mechanism we provide them is streams. And on first look at it, it seems possible to send a STREAM frame alongside a CONNECTION_CLOSE (that uses an APPLICATION_SPECIFIC_ERROR code). But it isn't that easy because streams could be blocked. On MAX_DATA, MAX_STREAM_DATA, or MAX_STREAM_ID. You would have to willingly violate flow control or stream limits to get the message through, and that increases the chances that the message would be discarded on receipt. |
Well, we now have connection and application close in the code, and it would cost more to merge them than to leave them separate. So I vote to leave that one alone. |
We went through the process of separating error codes into transport and
application error codes, and the frames exist to indicate which of those
spaces. Also, as Martin notes, we want to be able to use this for a
connection close, which means the transport has to know the semantics of
this message even if it is defined at the application level.
I don't see a strong argument for merging them.
…On Dec 3, 2017 5:39 PM, "Christian Huitema" ***@***.***> wrote:
Well, we now have connection and application close in the code, and it
would cost more to merge them than to leave them separate. So I vote to
leave that one alone.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#978 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKjg1K5LaKKBHPbaHCY7uX9ZlPF2czNHks5s801agaJpZM4QyCuV>
.
|
we went through the same process of splitting the error codes spaces as well, so I don't see a strong argument for merging them either |
OK, consensus seems to be to leave things as they are - closing this |
Since CONNECTION_CLOSE and APPLICATION_CLOSE have the same format, are already distinguishable by the different error code spaces, and have the same semantics, we could save a frame type number here.
The text was updated successfully, but these errors were encountered: