From 82529e614f71fbc36d9212377376a33f2d4220f0 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 5 Jun 2020 22:48:02 +0200 Subject: [PATCH] Issue #4855 - Occasional h2spec failures on CI Fixed notification of HTTP2Session.abort(), that must fail all the streams before failing the session. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 39 ++++++++++++------- .../org/eclipse/jetty/http2/HTTP2Stream.java | 2 +- .../org/eclipse/jetty/http2/api/Stream.java | 16 ++++++++ .../jetty/http2/frames/FailureFrame.java | 9 ++++- .../jetty/http2/generator/Generator.java | 2 +- ...nnectGenerator.java => NoOpGenerator.java} | 4 +- .../client/http/HttpReceiverOverHTTP2.java | 4 +- .../server/HTTP2ServerConnectionFactory.java | 6 ++- 8 files changed, 60 insertions(+), 22 deletions(-) rename jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/{DisconnectGenerator.java => NoOpGenerator.java} (92%) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 50b3ade73f8d..035c9dc04f14 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -498,14 +498,15 @@ public void onWindowUpdate(WindowUpdateFrame frame) public void onStreamFailure(int streamId, int error, String reason) { Callback callback = new ResetCallback(streamId, error, Callback.NOOP); - onStreamFailure(streamId, error, reason, callback); + Throwable failure = toFailure("Stream failure", error, reason); + onStreamFailure(streamId, error, reason, failure, callback); } - private void onStreamFailure(int streamId, int error, String reason, Callback callback) + private void onStreamFailure(int streamId, int error, String reason, Throwable failure, Callback callback) { IStream stream = getStream(streamId); if (stream != null) - stream.process(new FailureFrame(error, reason), callback); + stream.process(new FailureFrame(error, reason, failure), callback); else callback.succeeded(); } @@ -517,31 +518,48 @@ public void onConnectionFailure(int error, String reason) } protected void onConnectionFailure(int error, String reason, Callback callback) + { + Throwable failure = toFailure("Session failure", error, reason); + onFailure(error, reason, failure, new CloseCallback(error, reason, callback)); + } + + protected void abort(Throwable failure) + { + onFailure(ErrorCode.NO_ERROR.code, null, failure, new TerminateCallback(failure)); + } + + private void onFailure(int error, String reason, Throwable failure, Callback callback) { Collection streams = getStreams(); int count = streams.size(); - Callback countCallback = new CountingCallback(new CloseCallback(error, reason, callback), count + 1); + Callback countCallback = new CountingCallback(callback, count + 1); for (Stream stream : streams) { - onStreamFailure(stream.getId(), error, reason, countCallback); + onStreamFailure(stream.getId(), error, reason, failure, countCallback); } - IOException failure = new IOException(String.format("%s/%s", ErrorCode.toString(error, null), reason)); notifyFailure(this, failure, countCallback); } private void onClose(GoAwayFrame frame, Callback callback) { + int error = frame.getError(); + String reason = frame.tryConvertPayload(); + Throwable failure = toFailure("Session close", error, reason); Collection streams = getStreams(); int count = streams.size(); Callback countCallback = new CountingCallback(callback, count + 1); - String reason = frame.tryConvertPayload(); for (Stream stream : streams) { - onStreamFailure(stream.getId(), frame.getError(), reason, countCallback); + onStreamFailure(stream.getId(), error, reason, failure, countCallback); } notifyClose(this, frame, countCallback); } + private Throwable toFailure(String message, int error, String reason) + { + return new IOException(String.format("%s %s/%s", message, ErrorCode.toString(error, null), reason)); + } + @Override public void newStream(HeadersFrame frame, Promise promise, Stream.Listener listener) { @@ -1024,11 +1042,6 @@ private void terminate(Throwable cause) } } - protected void abort(Throwable failure) - { - notifyFailure(this, failure, new TerminateCallback(failure)); - } - public boolean isDisconnected() { return !endPoint.isOpen(); diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java index 0af903ede15c..f8360ab0d096 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java @@ -609,7 +609,7 @@ private void notifyFailure(Stream stream, FailureFrame frame, Callback callback) { try { - listener.onFailure(stream, frame.getError(), frame.getReason(), callback); + listener.onFailure(stream, frame.getError(), frame.getReason(), frame.getFailure(), callback); } catch (Throwable x) { diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java index 083bae9a1b0a..95d353bd54de 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java @@ -227,8 +227,24 @@ default boolean onIdleTimeout(Stream stream, Throwable x) * @param stream the stream * @param error the error code * @param reason the error reason, or null + * @param failure the failure * @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) + { + onFailure(stream, error, reason, callback); + } + + /** + *

Callback method invoked when the stream failed.

+ * + * @param stream the stream + * @param error the error code + * @param reason the error reason, or null + * @param callback the callback to complete when the failure has been handled + * @deprecated use {@link #onFailure(Stream, int, String, Throwable, Callback)} instead + */ + @Deprecated default void onFailure(Stream stream, int error, String reason, Callback callback) { callback.succeeded(); diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/FailureFrame.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/FailureFrame.java index 95c8dbadfbb6..ea6de570d603 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/FailureFrame.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/FailureFrame.java @@ -22,12 +22,14 @@ public class FailureFrame extends Frame { private final int error; private final String reason; + private final Throwable failure; - public FailureFrame(int error, String reason) + public FailureFrame(int error, String reason, Throwable failure) { super(FrameType.FAILURE); this.error = error; this.reason = reason; + this.failure = failure; } public int getError() @@ -39,4 +41,9 @@ public String getReason() { return reason; } + + public Throwable getFailure() + { + return failure; + } } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/Generator.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/Generator.java index f062041b7233..4a1b8c6fa457 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/Generator.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/Generator.java @@ -56,7 +56,7 @@ public Generator(ByteBufferPool byteBufferPool, int maxDynamicTableSize, int max this.generators[FrameType.WINDOW_UPDATE.getType()] = new WindowUpdateGenerator(headerGenerator); this.generators[FrameType.CONTINUATION.getType()] = null; // Never generated explicitly. this.generators[FrameType.PREFACE.getType()] = new PrefaceGenerator(); - this.generators[FrameType.DISCONNECT.getType()] = new DisconnectGenerator(); + this.generators[FrameType.DISCONNECT.getType()] = new NoOpGenerator(); this.dataGenerator = new DataGenerator(headerGenerator); } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/DisconnectGenerator.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/NoOpGenerator.java similarity index 92% rename from jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/DisconnectGenerator.java rename to jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/NoOpGenerator.java index e20c88d64cf7..1168c4197ea9 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/DisconnectGenerator.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/generator/NoOpGenerator.java @@ -21,9 +21,9 @@ import org.eclipse.jetty.http2.frames.Frame; import org.eclipse.jetty.io.ByteBufferPool; -public class DisconnectGenerator extends FrameGenerator +public class NoOpGenerator extends FrameGenerator { - public DisconnectGenerator() + public NoOpGenerator() { super(null); } diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java index 563ad4632a3d..854b6380538f 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java @@ -195,9 +195,9 @@ public boolean onIdleTimeout(Stream stream, Throwable x) } @Override - public void onFailure(Stream stream, int error, String reason, Callback callback) + public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) { - responseFailure(new IOException(String.format("%s/%s", ErrorCode.toString(error, null), reason))); + responseFailure(failure); callback.succeeded(); } diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java index 8d713ae98ea8..dc9f782c7498 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java @@ -34,6 +34,7 @@ import org.eclipse.jetty.http2.frames.ResetFrame; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.EofException; +import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.NegotiatingServerConnection.CipherDiscriminator; @@ -160,9 +161,10 @@ public void onReset(Stream stream, ResetFrame frame, Callback callback) } @Override - public void onFailure(Stream stream, int error, String reason, Callback callback) + public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) { - EofException failure = new EofException(String.format("Failure %s/%s", ErrorCode.toString(error, null), reason)); + if (!(failure instanceof QuietException)) + failure = new EofException(failure); onFailure(stream, failure, callback); }