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

Which stream error code for violating SETTINGS_MAX_CONCURRENT_STREAMS? #649

Closed
bradfitz opened this issue Nov 23, 2014 · 4 comments · Fixed by #652
Closed

Which stream error code for violating SETTINGS_MAX_CONCURRENT_STREAMS? #649

bradfitz opened this issue Nov 23, 2014 · 4 comments · Fixed by #652

Comments

@bradfitz
Copy link

"5.1.2 Stream Concurrency" says about SETTINGS_MAX_CONCURRENT_STREAMS:

"Endpoints MUST NOT exceed the limit set by their peer. An endpoint that receives a HEADERS frame that causes their advertised concurrent stream limit to be exceeded MUST treat this as a stream error (Section 5.4.2)"

But which stream error code should be used? Almost all other references to stream errors in the spec say "treated as a stream error of type XXX"

@bradfitz
Copy link
Author

Also, if this is a stream error, what happens if the client has a CONTINUATION already in flight right behind the current-streams-violating HEADERS frame? The server would then return a stream error to the HEADERS frame, and then see the CONTINUATION frame that's not associated with an in-progress header block.

I suppose a server should maintain enough state to remember that it sent a stream error on that stream ID and simply ignore any following CONTINUATION frames instead of returning a connection error?

bradfitz added a commit to bradfitz/http2 that referenced this issue Nov 23, 2014
But open question is which stream error code to use httpwg/http2-spec#649
@grmocg
Copy link
Contributor

grmocg commented Nov 23, 2014

The server knows that a continuation frame is coming as it is flagged in
the HEADERS frame.

Regardless, to ensure against corrupting the compression state, even if it
is thrown out, all HEADERS/CONTINUATION data must go through the
compression context unless you're closing the connection. This is even true
for stream IDs that are not in a valid state (though I'd hope in those
cases that the receiver would close the connection after immediately
sending an error).

On Sun, Nov 23, 2014 at 9:21 AM, Brad Fitzpatrick notifications@github.com
wrote:

Also, if this is a stream error, what happens if the client has a
CONTINUATION already in flight right behind the current-streams-violating
HEADERS frame? The server would then return a stream error to the HEADERS
frame, and then see the CONTINUATION frame that's not associated with an
in-progress header block.

I suppose a server should maintain enough state to remember that it sent a
stream error on that stream ID and simply ignore any following CONTINUATION
frames instead of returning a connection error?


Reply to this email directly or view it on GitHub
#649 (comment).

@bradfitz
Copy link
Author

@grmocg, good point. My tests don't cover the case of a CONTINUATION having valuable compression context needed for future valid streams. I will update the test suite I'm building.

bradfitz added a commit to bradfitz/http2 that referenced this issue Nov 24, 2014
Before it was enforced when processing the FRAME header, but as pointed
out on httpwg/http2-spec#649 , that means we were dropping any HPACK decoding
context in subsequent CONTINUATION frames.

Instead, move the check later to processHeaderBlockFragment (which
runs at END_HEADERS, whether in HEADERS or CONTINUATION).

And add a test using a CONTINUATION with decoder state to preserve.

This change also then is able to revert resetStream to its earlier
more paranoid behavior.
@mnot
Copy link
Member

mnot commented Nov 29, 2014

Discussed on-list; let's go with #652.

bradfitz added a commit to bradfitz/http2 that referenced this issue Dec 3, 2014
Choose the error code for violating SETTINGS_MAX_CONCURRENT_STREAMS depending
on whether the peer should know better.

See httpwg/http2-spec#649 and httpwg/http2-spec#652
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants