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.

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

Fixes #6208 - HTTP/2 max local stream count exceeded #6220

Merged
merged 2 commits into from Apr 29, 2021
Merged

Fixes #6208 - HTTP/2 max local stream count exceeded #6220

merged 2 commits into from Apr 29, 2021

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Apr 28, 2021

Backported from Jetty 10 the "new stream" event so that the Stream can be set early on the client's HttpChannelOverHTTP2.
In this way, when a HEADERS frame stalled due to TCP congestion is failed, the corresponding Stream is closed and the connection released to the pool, fixing the "max stream exceeded" issue.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

@sbordet
Fixes #6208 - HTTP/2 max local stream count exceeded
0b315d7 
Backported from Jetty 10 the "new stream" event so that the Stream can be set early on the client's `HttpChannelOverHTTP2`.
In this way, when a HEADERS frame stalled due to TCP congestion is failed, the corresponding Stream is closed and the connection released to the pool, fixing the "max stream exceeded" issue.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from gregw and lorban April 28, 2021 11:47
@sbordet sbordet linked an issue Apr 28, 2021 that may be closed by this pull request
HTTP/2 max local stream count exceeded #6208
Closed
@sbordet sbordet added this to In progress in Jetty 9.4.41 via automation Apr 28, 2021
@sbordet
Copy link
Contributor Author

sbordet commented Apr 28, 2021

@gregw @lorban see #6208 (comment) for an explanation of the issue.

The fix is as follows:

  • Backported from 10 the "new stream" event that happens as soon as a local stream is created. In this way we don't have to wait for the HEADERS frame to be written out to the network before having the Stream object available.
  • If the HEADERS frame is not written due to TCP congestion, and then the corresponding request times out, now the Stream is available to send a reset to the remote peer.
  • Sending a reset to the remote peer may be against the specification; I have introduced a commit flag to the stream that is set as soon as the first control frame for that stream is written to the network.
  • By checking the commit flag we can drop RST_STREAM frames to avoid violating the specification.

@sbordet
Issue #6208 - HTTP/2 max local stream count exceeded
967e13d 
Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
lorban
lorban approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

LGTM after peer review.

Jetty 9.4.41 automation moved this from In progress to Reviewer approved Apr 28, 2021
gregw
gregw reviewed Apr 28, 2021
View reviewed changes
jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java
@Override
public void commit()
{
committed = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the memory barrier on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flag is set and read from the flusher thread, so the memory barrier is guaranteed by IteratingCallback.

gregw
gregw approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Other than slight concern on memory barrier... looks OK.
Logic felt a bit special for RST, but then I think it is.

@sbordet sbordet merged commit 2f19c67 into jetty-9.4.x Apr 29, 2021
Jetty 9.4.41 automation moved this from Reviewer approved to Done Apr 29, 2021
@sbordet sbordet deleted the jetty-9.4.x-6208-max_streams_exceeded_tcp_congested branch April 29, 2021 07:31
sbordet added a commit that referenced this pull request Apr 29, 2021
@sbordet
Fixes #6208 - HTTP/2 max local stream count exceeded (#6220)
2cb2462 
Forward port of #6220 from jetty-9.4.x to jetty-10.0.x.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
(cherry-picked from commit 2f19c67)
@sbordet sbordet mentioned this pull request Apr 29, 2021
Merged
sbordet added a commit that referenced this pull request Apr 29, 2021
@sbordet
Fixes #6208 - HTTP/2 max local stream count exceeded (#6220)
7048db0 
Forward port of #6220 from jetty-9.4.x to jetty-10.0.x.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
(cherry-picked from commit 2f19c67)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw approved these changes

@lorban lorban lorban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Jetty 9.4.41
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP/2 max local stream count exceeded
3 participants
@sbordet @gregw @lorban