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

Jetty 9.4.x 4855 h2spec failures #4946

Merged
merged 10 commits into from
Jun 9, 2020
Merged

Jetty 9.4.x 4855 h2spec failures #4946

merged 10 commits into from
Jun 9, 2020

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jun 5, 2020

The bulk of the fix is in 8d60636.

sbordet added 4 commits June 5, 2020 19:20
@sbordet
Issue #4855 - Occasional h2spec failures on CI
e721717 
Code cleanups.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Issue #4855 - Occasional h2spec failures on CI
fd053aa 
In case of bad usage of the API, we don't want to close()
the stream but just fail the callback, because the stream
may be performing actions triggered by a legit API usage.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Issue #4855 - Occasional h2spec failures on CI
9dec284 
In case of a call to `AsyncListener.onError()`, applications may decide to call
AsyncContext.complete() and that would be a correct usage of the Servlet API.

This case was not well handled and was wrongly producing a WARN log with an
`IllegalStateException`.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Fixes #4855 - Occasional h2spec failures on CI
8d60636 
Completely rewritten `HttpTransportOverHTTP2.TransportCallback`.
The rewrite handles correctly asynchronous failures that now are executed
sequentially (and not concurrently) with writes.
If a write is in progress, the failure will just change the state and at the
end of the write a check on the state will determine what actions to take.

A session failure is now handled in HTTP2Session by first failing all the
streams - which notifies the Stream.Listeners - and then failing the session
- which notifies the Session.Listener.
The stream failures are executed concurrently by dispatching each one to a
different thread; this means that the stream failure callbacks are executed
concurrently (likely sending RST_STREAM frames).
The session failure callback is completed only when all the stream failure
callbacks have completed, to ensure that a GOAWAY frame is processed after
all the RST_STREAM frames.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested review from gregw and lorban June 5, 2020 17:49
@sbordet sbordet linked an issue Jun 5, 2020 that may be closed by this pull request
occasional h2spec failures on jenkins #4855
Closed
@sbordet
Copy link
Contributor Author

sbordet commented Jun 5, 2020

@gregw @lorban see #4855 (comment) for a description of the problem and the solution.

@sbordet
Issue #4855 - Occasional h2spec failures on CI
82529e6 
Fixed notification of HTTP2Session.abort(), that must fail all the streams
before failing the session.

Signed-off-by: Simone Bordet <[email protected]>
gregw
gregw reviewed Jun 8, 2020
View reviewed changes
...y-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
}
}
LOG.debug("Processing session failure on {}", getSession(), failure);
// All the streams have already been failed, just succeed the callback.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method still required? It doesn't override any contract?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet ?

gregw
gregw requested changes Jun 8, 2020
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.

I can see what it is doing, but it is hard to thoroughly review without more comments explaining each state transition and the races being faced. So can I get some more comments and then I'll review again

...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
@sbordet
Issue #4855 - Occasional h2spec failures on CI
42a5d27 
Added javadocs after review.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested a review from gregw June 8, 2020 08:37
lorban
lorban requested changes Jun 8, 2020
View reviewed changes
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated
}
}

private enum State
{
IDLE, WRITING, FAILED, TIMEOUT
IDLE, PRE_SEND, POST_SEND, SUCCEED, FAIL, FAILED
Copy link
Contributor

Choose a reason for hiding this comment

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

These states and their transitions should be javadoc'ed as it's not easy to grok what the FSM looks like by reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java
* @param callback the callback to complete when the failure has been handled
*/
default void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this new method's signature and default implementation surprising. I would have expected that the error and reason parameters to go away especially since the overriding implementations just ignore them. Plus the fact that the default implementation drops the failure in favor of calling the deprecated implementation instead of directly succeeding the callback also makes me scratch my head.

Maybe FailureFrame.getFailure() should return some sort of H2Exception that wraps the original exception as well as containing the reason and error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is at the HTTP/2 low-level API so our implementation may not use error and reason but users of these APIs may.
However, there are cases where we still have a failure but not an associated error (e.g. idle timeout and aborts).

gregw
gregw requested changes Jun 8, 2020
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.

Big block comments are good, but names are bad and more inline comments needed

...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Outdated Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
@sbordet sbordet requested review from lorban and gregw June 8, 2020 10:20
sbordet added 2 commits June 8, 2020 12:42
@sbordet
Issue #4855 - Occasional h2spec failures on CI
4c9bd59 
More comments after review.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Issue #4855 - Occasional h2spec failures on CI
ee51839 
Updates after review.

Signed-off-by: Simone Bordet <[email protected]>
gregw
gregw requested changes Jun 8, 2020
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.

still a few things...

...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
...-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java Outdated Show resolved Hide resolved
lorban
lorban approved these changes Jun 8, 2020
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 once the naming has been sorted out.

@sbordet
Issue #4855 - Occasional h2spec failures on CI
56d3a6e 
Updates after review.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested a review from gregw June 8, 2020 14:15
@gregw
Copy link
Contributor

gregw commented Jun 8, 2020

@sbordet CI failed

@sbordet
Issue #4855 - Occasional h2spec failures on CI
26ad585 
Fixed reset() to remember the failure.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet merged commit 56bda1b into jetty-9.4.x Jun 9, 2020
@sbordet sbordet deleted the jetty-9.4.x-4855-h2spec_failures branch June 9, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorban lorban lorban approved these changes

@gregw gregw gregw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

occasional h2spec failures on jenkins
3 participants
@sbordet @gregw @lorban