Skip to content

h2spec: handling PRIORITY frame that depend on itself #1752

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.

Sign up for GitHub

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

Jump to bottom
Merged
maskit merged 1 commit into from
May 6, 2017
Merged

h2spec: handling PRIORITY frame that depend on itself #1752

maskit merged 1 commit into from
May 6, 2017

Conversation

@zizhong
Copy link
Member

@zizhong zizhong commented May 2, 2017

The issue reported by h2spec is

  5. Streams and Multiplexing
    5.3. Stream Priority
      5.3.1. Stream Dependencies
        × 2: Sends PRIORITY frame that depend on itself
          -> The endpoint MUST treat this as a stream error of type PROTOCOL_ERROR.
             Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                       RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                       Connection closed
               Actual: Timeout

With this fix,

Hypertext Transfer Protocol Version 2 (HTTP/2)
  5. Streams and Multiplexing
    5.3. Stream Priority
      5.3.1. Stream Dependencies
             [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
             [recv] SETTINGS Frame (length:12, flags:0x00, stream_id:0)
             [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
             [recv] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
             [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
             [send] PRIORITY Frame (length:5, flags:0x00, stream_id:1)
             [recv] Connection closed
        ✔ 2: Sends PRIORITY frame that depend on itself

Finished in 0.0501 seconds
1 tests, 1 passed, 0 skipped, 0 failed

@zizhong zizhong mentioned this pull request May 2, 2017
Closed
@masaori335 masaori335 requested review from masaori335 and maskit May 2, 2017 23:49
@masaori335 masaori335 added this to the 8.0.0 milestone May 2, 2017
@masaori335
Copy link
Contributor

masaori335 commented May 2, 2017

[approve ci]

@atsci
Copy link

atsci commented May 2, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/356/

@atsci
Copy link

atsci commented May 2, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2038/

@atsci
Copy link

atsci commented May 2, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/469/

@atsci
Copy link

atsci commented May 3, 2017

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/340/

@atsci
Copy link

atsci commented May 3, 2017

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/345/

@atsci
Copy link

atsci commented May 3, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1933/

@atsci
Copy link

atsci commented May 3, 2017

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/602/

maskit
maskit requested changes May 3, 2017
View reviewed changes
Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should treat it as a connection error.

proxy/http2/Http2ConnectionState.cc Outdated

// A stream cannot depend on itself. An endpoint MUST treat this as a stream error of type PROTOCOL_ERROR.
if (stream_id == priority.stream_dependency) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
Copy link
Member

@maskit maskit May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says treat it as a stream error, so please use HTTP2_ERROR_CLASS_STREAM instead.

Copy link
Member Author

@zizhong zizhong May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maskit Thanks! Fixed.

@zizhong zizhong force-pushed the http2spec_5.3.2 branch from 4a089fd to 48f3b1b Compare May 3, 2017 18:18
@maskit
Copy link
Member

maskit commented May 6, 2017

Thanks.

@maskit maskit merged commit 0098932 into apache:master May 6, 2017
@zwoop
Copy link
Contributor

zwoop commented May 7, 2017

@maskit Do we want this in 7.1.0 ?

@maskit
Copy link
Member

maskit commented May 7, 2017

@zwoop I think we should. Masaori says it might cause memory leak.

@zwoop zwoop modified the milestones: 7.1.0, 8.0.0 May 9, 2017
@zwoop
Copy link
Contributor

zwoop commented May 9, 2017

Cherry picked to 7.1.x, building and testing on docs now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit approved these changes

@masaori335 masaori335 masaori335 approved these changes

Assignees

@zizhong zizhong

Labels

HTTP/2

Projects

None yet

Milestone

7.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@zizhong @masaori335 @atsci @maskit @zwoop