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

RST_STREAM breaks HPACK #176

Closed
MikeBishop opened this issue Jan 19, 2017 · 23 comments
Closed

RST_STREAM breaks HPACK #176

MikeBishop opened this issue Jan 19, 2017 · 23 comments
Labels
-http 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

@MikeBishop
Copy link
Contributor

@martinthomson noticed an issue in the QPACK draft that's equally true with HPACK in -01:

If you RST_STREAM a stream with a missing QUIC STREAM frame that contains an HPACK frame (HEADERS, PUSH_PROMISE), that missing frame never gets retransmitted. That, in turn, means that no future HPACK frame can be interpreted, ever, because you're missing part of your history.

Oops.

My first inclination is to change the HTTP draft to say that message control streams MUST NOT be reset. If you don't want the message body, reset the data stream, but you MUST read the message headers. If, for some reason, you cannot do so, then terminate the connection.

Anyone see a barrier to that (other than the price of ginormous headers)?

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Jan 19, 2017
@MikeBishop
Copy link
Contributor Author

(By the way, attempting to describe this issue highlights #128 very nicely.)

@LPardue
Copy link
Member

LPardue commented Jan 19, 2017

I'm don't think I appreciate the full scale of this problem or maybe I have a misunderstanding. However I wonder about the relation of RST_STREAM to promised streams?

Consider an "over eager" server that is configured to push everything and assume some level of serial processing. Your proposal implies that the client may only cancel data streams, requiring the server to allocate resource to generation of response headers (that may be a waste). A smarter implementation could read the promised data stream reset and infer that it should give up on the push-related response altogether; perhaps the server could cancel it's own control stream?

@ianswett
Copy link
Contributor

I thought one purpose of giving headers its own stream was so you can't reset the headers stream, but you can the body stream?

So yes, I'd suggest you change the draft to say message control streams MUST NOT be reset.

@krasic
Copy link

krasic commented Jan 19, 2017

There is a wrinkle I encountered, you still need to allow QUIC_REFUSED_STREAM form of reset. Due to this, I think it makes sense to note that HTTP stream pairs should be created together, and if necessary REFUSED together. This is what I'm doing in my current implementation of HoL avoiding HPACK.

@krasic
Copy link

krasic commented Jan 19, 2017

@LPardue - I think rfc7540 10.5 addresses the over eager server case. A PUSH_PROMISE comes from an associated stream, which must have already had resources allocated.

"The number of PUSH_PROMISE frames is not constrained in the same
fashion. A client that accepts server push SHOULD limit the number
of streams it allows to be in the "reserved (remote)" state. An
excessive number of server push streams can be treated as a stream
error (Section 5.4.2) of type ENHANCE_YOUR_CALM."

@LPardue
Copy link
Member

LPardue commented Jan 19, 2017 via email

@krasic
Copy link

krasic commented Jan 19, 2017

Well, at least in current Chromium, an excessive number of push promises will lead to the promised streams being reset with QUIC_REFUSED_STREAM. I am the author of that particular bit of code.

https://codesearch.chromium.org/chromium/src/net/quic/core/quic_client_session_base.cc?rcl=1484842630&l=97

@martinthomson
Copy link
Member

I don't think that the answer is blocking stream resets. That's taking a capability away from the transport that it needs to have independent of whatever needs the application protocol has. Instead, we need to devise a way to ensure that missing references are properly counted.

In HTTP/2 we insisted that HPACK frames be processed even after a reset. That doesn't work here because they might not ever arrive and we can't insist that they be repaired either because the transport isn't aware of that.

My only idea for solving this is a bad one: create a new frame type that is sent on the connection control stream that includes a list of tuples comprising a stream identifier, an index number and a count. That frame would only be sent for reset streams and it would include a total value for each stream. The problem that causes is that you need to maintain a per-stream counter of references to entries in the table, which - even if the count is only needed for active streams - really jacks up the per-entry overheads on the table.

Note that we can't even use QUIC ACK frames to help determine if a HEADERS frame was processed, because that only indicates that the data was received, not processed. A reset stream will cause buffered data to be discarded, even if it was successfully delivered.

As I think @krasic is pointing out, refusing a stream is a good way to indicate that processing didn't happen on a stream. We could infer that to mean that the references to the header table didn't happen either. That doesn't solve the problem though. On the contrary, it probably makes things worse by increasing complexity.

@LPardue
Copy link
Member

LPardue commented Jan 19, 2017 via email

@martinthomson
Copy link
Member

Exceeding the concurrent stream limit is a straight up protocol error, which needs to be handled by blowing the connection up.

I am thinking about this in more an abstract "who gets to kill streams" framework. We might decide that only the application protocol decides when streams are killed (for anything other than protocol violations, that is). If we get to that point, then the idea that we can prohibit killing of control streams might be OK. It's a pretty extreme reaction to what should be a solveable problem though and I'd like to at least try to find alternative options.

@martinthomson
Copy link
Member

Oh @LPardue, with respect to the PUSH_PROMISE limiting issue, that's not really relevant to this discussion. The server can promise an unbounded number of pushes and the client has to either suffer it or kill the connection. If you think that's a bad design (I am inclined to agree that it's not great), then please open another issue.

@LPardue
Copy link
Member

LPardue commented Jan 19, 2017

Just to clarify one point I was trying to make for my use case (annoying pushy server). Lets assume the client has a load of resources cached.

In vanilla HTTP/2, the client could send RST_STREAM for all promised streams relating to things in its cache. Meaning it doesn't read headers or data (perhaps, depending on timings?)

In the QUIC (as defined here), the client can only cancel the data stream. Making the server and client handle headers just for the sake of a compression layer?

I don't think it warrants another issue creating, unless there's strong opinion to do so. Perhaps I can put a pin in this particular hypothetical issue until the real 176 issue is shaken out.

@martinthomson
Copy link
Member

Oh, I see what you are getting at: you are saying that it should be perfectly OK to pre-emptively reset a stream that never actually opens. I agree, but you don't ever know if it hasn't been started or not. That actually suggests (to me at least), that #174 might contain the answer to that problem.

@krasic
Copy link

krasic commented Jan 20, 2017

I don't fully grok #174 yet.

That said, the chromium QUIC code does indeed support pre-emptive reset of streams, making sure that such streams can be reset before they are opened, and if necessary prevents them from opening after the reset. This was indeed necessary for refusing excessive promised streams, and also for enforcing shared fate as I mentioned above.

@krasic
Copy link

krasic commented Jan 20, 2017

@martinthomson: re above I am thinking that if control stream is refused, it will try again later (on a new stream), so from HPACK view, the treatment is still in the spirit of control streams never get reset---any headers processed by the encoder must eventually be processed by the decoder, although refused streams may add delay to that process. A key to making it tractable, is the point I've made that either both control and data are accepted or both refused.

@MikeBishop
Copy link
Contributor Author

@krasic: At least in our implementation, "refused" meant that the HTTP request would be retried. That's a different thing than saying the exact HEADERS frame MUST be cached at the HTTP layer and re-sent on a different stream later. What if you subsequently decide you don't need that data and don't want to issue the request any more?

I'd like to see a solution where we can either guarantee delivery despite/before the RST, or make the header compression scheme resilient to lost packets. @martinthomson's direction is somewhat promising, but it's worse than that supposes. A RST_STREAM (or REQUEST_RST, if we take #171) could arrive after the app-layer has already fully sent and FIN'd its portion of the message. Will the app layer even know if a stream is RST after it's done with it? If so, when does the app know that the stream will definitely not be reset and therefore the per-stream state can be discarded? Also, what if it's the delete itself that went missing, so the decoder doesn't know it's waiting on a particular number of references?

@MikeBishop
Copy link
Contributor Author

Total top-of-my-head idea, possibly very bad side effects: What if the app can mark certain writes (in HTTP's case, anything with HPACK/QPACK data) as required-to-deliver? The RST_STREAM could include both required-to-deliver and final offsets. Final offset is still used for flow control, so sender pays for everything even if they don't RTD it.

The sender would continue to retransmit everything before the required-to-deliver offset, and the receiver would surface the RST to the receiving app at the first gap after the required-to-deliver offset. (This also implies that only the sender can RST their direction, in the vein of REQUEST_RST.)

@MikeBishop
Copy link
Contributor Author

Note that this is a more general problem -- if data gets discarded by the transport without tearing down the connection, maintaining shared state at the application layer becomes tricky. You can concentrate your state in a few "special" streams and say that if those streams ever get RST, it's a fatal error (we should have text to that effect about Stream 1 somewhere, incidentally!), or you can create a mechanism to differentiate really-reliable and mostly-reliable data.

@krasic
Copy link

krasic commented Jan 21, 2017

@MikeBishop but refused is explicitly defined in a way that it is safe to retry, precisely because there is no way of leaking into application level state:

The REFUSED_STREAM error code can be included in a RST_STREAM
frame to indicate that the stream is being closed prior to any
processing having occurred. Any request that was sent on the
reset stream can be safely retried.

Aside from refused, I think that there is any other form of reset needed for the headers/control streams. If we stick to that, the complexity for HPACK can be kept way down.

@ddragana
Copy link
Contributor

Some thoughts:

What about solving this in the application. HPACK does not get updated until all header packets for a stream are received. This probably must required to update HPACK in stream order: HPACK must be updated with headers from stream x before it can be updated with the headers from stream x+1 (if stream x and x+1 are not reset).

The efficiency of HPACK will be lower though.

Sorry, this was a quick suggestion and I have not go through all detail and side effects.

@krasic
Copy link

krasic commented Jan 23, 2017

@ddragana I think stream order won't work, for instance because of trailers (used by grpc). Another example would be proxys, where upstream services may have different response times, so forwarded responses will not match the order of incoming requests.

@ddragana
Copy link
Contributor

that is true. it was just not thought through.

@MikeBishop
Copy link
Contributor Author

Also, PUSH_PROMISE and HEADERS on streams where the server is responding will be out of sequence with the HEADERS frames on the push streams themselves.

@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http 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

Successfully merging a pull request may close this issue.

7 participants