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

http2: Add handling stream level error #663

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tatsuhiro-t
Contributor

tatsuhiro-t commented Feb 17, 2016

Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped. This is
undesirable since there may be other streams multiplexed and they are
very much fine. This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open. The existing
CURLE_HTTP2 means connection error in general.

Discussed in GH-659

tatsuhiro-t added some commits Feb 17, 2016

http2: Add handling stream level error
Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped.  This is
undesirable since there may be other streams multiplexed and they are
very much fine.  This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open.  The existing
CURLE_HTTP2 means connection error in general.

Ref: #659
Ref: #663
http2: Process paused data first before tear down http2 session
This commit ensures that data from network are processed before HTTP/2
session is terminated.  This is achieved by pausing nghttp2 whenever
different stream than current easy handle receives data.

This commit also fixes the bug that sometimes processing hangs when
multiple HTTP/2 streams are multiplexed.
http2: Ensure that http2_handle_stream_close is called
This commit ensures that streams which was closed in on_stream_close
callback gets passed to http2_handle_stream_close.  Previously, this
might not happen.  To achieve this, we increment drain property to
forcibly call recv function for that stream.

To more accurately check that we have no pending event before shutting
down HTTP/2 session, we sum up drain property into
http_conn.drain_total.  We only shutdown session if that value is 0.

With this commit, when stream was closed before reading response
header fields, error code CURLE_HTTP2_STREAM is returned even if
HTTP/2 level error is NO_ERROR.  This signals the upper layer that
stream was closed by error just like TCP connection close in HTTP/1.
http2: Don't increment drain when one headr field is received
Sicne we write header field in temporary location, not in the memory
that upper layer provides, incrementing drain should not happen.

@bagder bagder added the HTTP/2 label Feb 24, 2016

@jay jay self-assigned this Mar 1, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 2, 2016

Member

@jay, how's your plan for this PR? Sounds like something we want to merge...

Member

bagder commented Apr 2, 2016

@jay, how's your plan for this PR? Sounds like something we want to merge...

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 6, 2016

Member

@jay, how's your plan for this PR? Sounds like something we want to merge...

Yes. I ran out of time to review it, and I planned to revisit it after the release but I got caught up in other things. If I can't get to it this week I'll relinquish my assignment. It looks like I have to add some overflow detection as well, as Tatsuhiro noted in his last comment in #659.

Member

jay commented Apr 6, 2016

@jay, how's your plan for this PR? Sounds like something we want to merge...

Yes. I ran out of time to review it, and I planned to revisit it after the release but I got caught up in other things. If I can't get to it this week I'll relinquish my assignment. It looks like I have to add some overflow detection as well, as Tatsuhiro noted in his last comment in #659.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 6, 2016

Member

Also this may affect #750, I'm not sure...

Member

jay commented Apr 6, 2016

Also this may affect #750, I'm not sure...

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 6, 2016

Member

I don't think it affects #750, as that's not actually a http2 error, stream or otherwise.

Member

bagder commented Apr 6, 2016

I don't think it affects #750, as that's not actually a http2 error, stream or otherwise.

jay added a commit to jay/curl that referenced this pull request Apr 7, 2016

http2: Add handling stream level error
Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped.  This is
undesirable since there may be other streams multiplexed and they are
very much fine.  This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open.  The existing
CURLE_HTTP2 means connection error in general.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Add handling stream level error
Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped.  This is
undesirable since there may be other streams multiplexed and they are
very much fine.  This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open.  The existing
CURLE_HTTP2 means connection error in general.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Process paused data first before tear down http2 session
This commit ensures that data from network are processed before HTTP/2
session is terminated.  This is achieved by pausing nghttp2 whenever
different stream than current easy handle receives data.

This commit also fixes the bug that sometimes processing hangs when
multiple HTTP/2 streams are multiplexed.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Ensure that http2_handle_stream_close is called
This commit ensures that streams which was closed in on_stream_close
callback gets passed to http2_handle_stream_close.  Previously, this
might not happen.  To achieve this, we increment drain property to
forcibly call recv function for that stream.

To more accurately check that we have no pending event before shutting
down HTTP/2 session, we sum up drain property into
http_conn.drain_total.  We only shutdown session if that value is 0.

With this commit, when stream was closed before reading response
header fields, error code CURLE_HTTP2_STREAM is returned even if
HTTP/2 level error is NO_ERROR.  This signals the upper layer that
stream was closed by error just like TCP connection close in HTTP/1.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Don't increment drain when one headr field is received
Sicne we write header field in temporary location, not in the memory
that upper layer provides, incrementing drain should not happen.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Improve header parsing
- Allow spaces in the path.

- Make sure each header line ends in \r\n. This fixes an out of bounds.

- Disallow header continuation lines until we decide what to do.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Don't increment drain when one header field is received
Sicne we write header field in temporary location, not in the memory
that upper layer provides, incrementing drain should not happen.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Improve header parsing
- Allow spaces in the path.

- Make sure each header line ends in \r\n. This fixes an out of bounds.

- Disallow header continuation lines until we decide what to do.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 8, 2016

http2: Improve header parsing
- Allow spaces in the path.

- Make sure each header line ends in \r\n. This fixes an out of bounds.

- Disallow header continuation lines until we decide what to do.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

http2: Add handling stream level error
Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped.  This is
undesirable since there may be other streams multiplexed and they are
very much fine.  This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open.  The existing
CURLE_HTTP2 means connection error in general.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

http2: Process paused data first before tear down http2 session
This commit ensures that data from network are processed before HTTP/2
session is terminated.  This is achieved by pausing nghttp2 whenever
different stream than current easy handle receives data.

This commit also fixes the bug that sometimes processing hangs when
multiple HTTP/2 streams are multiplexed.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

http2: Ensure that http2_handle_stream_close is called
This commit ensures that streams which was closed in on_stream_close
callback gets passed to http2_handle_stream_close.  Previously, this
might not happen.  To achieve this, we increment drain property to
forcibly call recv function for that stream.

To more accurately check that we have no pending event before shutting
down HTTP/2 session, we sum up drain property into
http_conn.drain_total.  We only shutdown session if that value is 0.

With this commit, when stream was closed before reading response
header fields, error code CURLE_HTTP2_STREAM is returned even if
HTTP/2 level error is NO_ERROR.  This signals the upper layer that
stream was closed by error just like TCP connection close in HTTP/1.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

http2: Don't increment drain when one header field is received
Sicne we write header field in temporary location, not in the memory
that upper layer provides, incrementing drain should not happen.

Ref: curl#659
Ref: curl#663

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

jay added a commit to jay/curl that referenced this pull request Apr 10, 2016

http2: Improve header parsing
- Allow spaces in the path.

- Make sure each header line ends in \r\n. This fixes an out of bounds.

- Disallow header continuation lines until we decide what to do.

Ref: curl#659
Ref: curl#663

jay added a commit that referenced this pull request Apr 12, 2016

http2: Add handling stream level error
Previously, when a stream was closed with other than NGHTTP2_NO_ERROR
by RST_STREAM, underlying TCP connection was dropped.  This is
undesirable since there may be other streams multiplexed and they are
very much fine.  This change introduce new error code
CURLE_HTTP2_STREAM, which indicates stream error that only affects the
relevant stream, and connection should be kept open.  The existing
CURLE_HTTP2 means connection error in general.

Ref: #659
Ref: #663

jay added a commit that referenced this pull request Apr 12, 2016

jay added a commit that referenced this pull request Apr 12, 2016

http2: Process paused data first before tear down http2 session
This commit ensures that data from network are processed before HTTP/2
session is terminated.  This is achieved by pausing nghttp2 whenever
different stream than current easy handle receives data.

This commit also fixes the bug that sometimes processing hangs when
multiple HTTP/2 streams are multiplexed.

Ref: #659
Ref: #663

jay added a commit that referenced this pull request Apr 12, 2016

http2: Ensure that http2_handle_stream_close is called
This commit ensures that streams which was closed in on_stream_close
callback gets passed to http2_handle_stream_close.  Previously, this
might not happen.  To achieve this, we increment drain property to
forcibly call recv function for that stream.

To more accurately check that we have no pending event before shutting
down HTTP/2 session, we sum up drain property into
http_conn.drain_total.  We only shutdown session if that value is 0.

With this commit, when stream was closed before reading response
header fields, error code CURLE_HTTP2_STREAM is returned even if
HTTP/2 level error is NO_ERROR.  This signals the upper layer that
stream was closed by error just like TCP connection close in HTTP/1.

Ref: #659
Ref: #663

jay added a commit that referenced this pull request Apr 12, 2016

http2: Don't increment drain when one header field is received
Sicne we write header field in temporary location, not in the memory
that upper layer provides, incrementing drain should not happen.

Ref: #659
Ref: #663

jay added a commit that referenced this pull request Apr 12, 2016

jay added a commit that referenced this pull request Apr 12, 2016

http2: Improve header parsing
- Error if a header line is larger than supported.

- Warn if cumulative header line length may be larger than supported.

- Allow spaces when parsing the path component.

- Make sure each header line ends in \r\n. This fixes an out of bounds.

- Disallow header continuation lines until we decide what to do.

Ref: #659
Ref: #663

jay added a commit that referenced this pull request Apr 12, 2016

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 14, 2016

Member

Landed in b2a0376...3f57880, any discussion should continue in #659. Thanks!

Member

jay commented Apr 14, 2016

Landed in b2a0376...3f57880, any discussion should continue in #659. Thanks!

@jay jay closed this Apr 14, 2016

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