From 62e17da2d02e41ee14148e4d324b60b546e836f3 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 6 Nov 2020 16:13:12 +0100 Subject: [PATCH 01/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Moved stream "close" event after returning from listener methods. Signed-off-by: Simone Bordet --- .../eclipse/jetty/http2/client/HTTP2ClientSession.java | 4 ++++ .../main/java/org/eclipse/jetty/http2/HTTP2Stream.java | 9 +++------ .../eclipse/jetty/http2/server/HTTP2ServerSession.java | 7 +++++++ 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientSession.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientSession.java index 8c8a2bd9dd9..794531ff517 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientSession.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientSession.java @@ -19,6 +19,7 @@ package org.eclipse.jetty.http2.client; import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.CloseState; import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.FlowControlStrategy; import org.eclipse.jetty.http2.HTTP2Session; @@ -62,7 +63,10 @@ public class HTTP2ClientSession extends HTTP2Session else { stream.process(frame, Callback.NOOP); + boolean closed = stream.updateClose(frame.isEndStream(), CloseState.Event.RECEIVED); notifyHeaders(stream, frame); + if (closed) + removeStream(stream); } } else 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 e427057819b..b00d4ca87ed 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 @@ -328,9 +328,6 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa dataLength = length >= 0 ? length : Long.MIN_VALUE; } - if (updateClose(frame.isEndStream(), CloseState.Event.RECEIVED)) - session.removeStream(this); - callback.succeeded(); } @@ -371,10 +368,10 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa } } - if (updateClose(frame.isEndStream(), CloseState.Event.RECEIVED)) - session.removeStream(this); - + boolean closed = updateClose(frame.isEndStream(), CloseState.Event.RECEIVED); notifyData(this, frame, callback); + if (closed) + session.removeStream(this); } private void onReset(ResetFrame frame, Callback callback) diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java index 1a5c7079b3a..6e87bf17675 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Map; import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.CloseState; import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.FlowControlStrategy; import org.eclipse.jetty.http2.HTTP2Session; @@ -109,8 +110,11 @@ public class HTTP2ServerSession extends HTTP2Session implements ServerParser.Lis { onStreamOpened(stream); stream.process(frame, Callback.NOOP); + boolean closed = stream.updateClose(frame.isEndStream(), CloseState.Event.RECEIVED); Stream.Listener listener = notifyNewStream(stream, frame); stream.setListener(listener); + if (closed) + removeStream(stream); } } } @@ -129,7 +133,10 @@ public class HTTP2ServerSession extends HTTP2Session implements ServerParser.Lis if (stream != null) { stream.process(frame, Callback.NOOP); + boolean closed = stream.updateClose(frame.isEndStream(), CloseState.Event.RECEIVED); notifyHeaders(stream, frame); + if (closed) + removeStream(stream); } else { From 0dc28940021755f79afad9930cea6d0437f5199a Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 8 Nov 2020 15:22:55 +0100 Subject: [PATCH 02/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Refactored reset code in a single place: HTTP2Session.reset(). Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 28 +++++++++---------- .../org/eclipse/jetty/http2/HTTP2Stream.java | 3 +- 2 files changed, 14 insertions(+), 17 deletions(-) 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 fb3a8e1f46a..015507ecdb6 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 @@ -278,7 +278,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio // otherwise other requests will be stalled. flowControl.onDataConsumed(this, null, flowControlLength); if (isStreamClosed(streamId)) - reset(new ResetFrame(streamId, ErrorCode.STREAM_CLOSED_ERROR.code), callback); + reset(null, new ResetFrame(streamId, ErrorCode.STREAM_CLOSED_ERROR.code), callback); else onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "unexpected_data_frame", callback); } @@ -505,7 +505,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio int streamSendWindow = stream.updateSendWindow(0); if (MathUtils.sumOverflows(streamSendWindow, windowDelta)) { - reset(new ResetFrame(streamId, ErrorCode.FLOW_CONTROL_ERROR.code), Callback.NOOP); + reset(stream, new ResetFrame(streamId, ErrorCode.FLOW_CONTROL_ERROR.code), Callback.NOOP); } else { @@ -642,9 +642,16 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio control(null, callback, frame); } - protected void reset(ResetFrame frame, Callback callback) + void reset(IStream stream, ResetFrame frame, Callback callback) { - control(getStream(frame.getStreamId()), callback, frame); + control(stream, Callback.from(() -> + { + if (stream != null) + { + stream.close(); + removeStream(stream); + } + }, callback), frame); } /** @@ -810,7 +817,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (maxCount >= 0 && remoteCount - remoteClosing >= maxCount) { updateLastRemoteStreamId(streamId); - reset(new ResetFrame(streamId, ErrorCode.REFUSED_STREAM_ERROR.code), Callback.NOOP); + reset(null, new ResetFrame(streamId, ErrorCode.REFUSED_STREAM_ERROR.code), Callback.NOOP); return null; } if (remoteStreamCount.compareAndSet(encoded, remoteCount + 1, remoteClosing)) @@ -1331,15 +1338,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio removeStream(stream); break; } - case RST_STREAM: - { - if (stream != null) - { - stream.close(); - removeStream(stream); - } - break; - } case PUSH_PROMISE: { // Pushed streams are implicitly remotely closed. @@ -1568,7 +1566,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private void complete() { - reset(new ResetFrame(streamId, error), getCallback()); + reset(getStream(streamId), new ResetFrame(streamId, error), getCallback()); } } 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 b00d4ca87ed..d0b108230ff 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 @@ -21,7 +21,6 @@ package org.eclipse.jetty.http2; import java.io.EOFException; import java.io.IOException; import java.nio.channels.WritePendingException; -import java.util.Collections; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; @@ -144,7 +143,7 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa localReset = true; failure = new EOFException("reset"); } - session.frames(this, Collections.singletonList(frame), callback); + ((HTTP2Session)session).reset(this, frame, callback); } private boolean startWrite(Callback callback) From 206050df7ff2c8d8126310fdad1654a3204d7db4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 8 Nov 2020 15:28:38 +0100 Subject: [PATCH 03/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Refactored push code in a single place: HTTP2Session.push(). Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) 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 015507ecdb6..6c038817946 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 @@ -624,7 +624,17 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public void push(IStream stream, Promise promise, PushPromiseFrame frame, Stream.Listener listener) { - streamCreator.push(frame, promise, listener); + streamCreator.push(frame, new Promise.Wrapper(promise) + { + @Override + public void succeeded(Stream pushedStream) + { + // Pushed streams are implicitly remotely closed. + // They are closed when sending an end-stream DATA frame. + ((IStream)pushedStream).updateClose(true, CloseState.Event.RECEIVED); + super.succeeded(pushedStream); + } + }, listener); } @Override @@ -1338,13 +1348,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio removeStream(stream); break; } - case PUSH_PROMISE: - { - // Pushed streams are implicitly remotely closed. - // They are closed when sending an end-stream DATA frame. - stream.updateClose(true, CloseState.Event.RECEIVED); - break; - } case GO_AWAY: { // We just sent a GO_AWAY, only shutdown the From 044052d717c070533493d8ba658a1d211dcc5630 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 8 Nov 2020 22:22:25 +0100 Subject: [PATCH 04/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Code cleanups: removed unnecessary final modifiers and fixed code warnings. Signed-off-by: Simone Bordet --- .../http2/client/FlowControlStrategyTest.java | 78 +++++++++---------- 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java index ba20dd08d7f..5d7a156e11d 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java @@ -137,10 +137,10 @@ public abstract class FlowControlStrategyTest @Test public void testWindowSizeUpdates() throws Exception { - final CountDownLatch prefaceLatch = new CountDownLatch(1); - final CountDownLatch stream1Latch = new CountDownLatch(1); - final CountDownLatch stream2Latch = new CountDownLatch(1); - final CountDownLatch settingsLatch = new CountDownLatch(1); + CountDownLatch prefaceLatch = new CountDownLatch(1); + CountDownLatch stream1Latch = new CountDownLatch(1); + CountDownLatch stream2Latch = new CountDownLatch(1); + CountDownLatch settingsLatch = new CountDownLatch(1); start(new ServerSessionListener.Adapter() { @Override @@ -233,11 +233,11 @@ public abstract class FlowControlStrategyTest // then we change the window to 512 B. At this point, the client // must stop sending data (although the initial window allows it). - final int size = 512; + int size = 512; // We get 3 data frames: the first of 1024 and 2 of 512 each // after the flow control window has been reduced. - final CountDownLatch dataLatch = new CountDownLatch(3); - final AtomicReference callbackRef = new AtomicReference<>(); + CountDownLatch dataLatch = new CountDownLatch(3); + AtomicReference callbackRef = new AtomicReference<>(); start(new ServerSessionListener.Adapter() { @Override @@ -276,7 +276,7 @@ public abstract class FlowControlStrategyTest }); // Two SETTINGS frames, the initial one and the one we send from the server. - final CountDownLatch settingsLatch = new CountDownLatch(2); + CountDownLatch settingsLatch = new CountDownLatch(2); Session session = newClient(new Session.Listener.Adapter() { @Override @@ -313,9 +313,9 @@ public abstract class FlowControlStrategyTest @Test public void testServerFlowControlOneBigWrite() throws Exception { - final int windowSize = 1536; - final int length = 5 * windowSize; - final CountDownLatch settingsLatch = new CountDownLatch(2); + int windowSize = 1536; + int length = 5 * windowSize; + CountDownLatch settingsLatch = new CountDownLatch(2); start(new ServerSessionListener.Adapter() { @Override @@ -350,13 +350,13 @@ public abstract class FlowControlStrategyTest assertTrue(settingsLatch.await(5, TimeUnit.SECONDS)); - final CountDownLatch dataLatch = new CountDownLatch(1); - final Exchanger exchanger = new Exchanger<>(); + CountDownLatch dataLatch = new CountDownLatch(1); + Exchanger exchanger = new Exchanger<>(); MetaData.Request metaData = newRequest("GET", new HttpFields()); HeadersFrame requestFrame = new HeadersFrame(metaData, null, true); session.newStream(requestFrame, new Promise.Adapter<>(), new Stream.Listener.Adapter() { - private AtomicInteger dataFrames = new AtomicInteger(); + private final AtomicInteger dataFrames = new AtomicInteger(); @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -407,10 +407,10 @@ public abstract class FlowControlStrategyTest @Test public void testClientFlowControlOneBigWrite() throws Exception { - final int windowSize = 1536; - final Exchanger exchanger = new Exchanger<>(); - final CountDownLatch settingsLatch = new CountDownLatch(1); - final CountDownLatch dataLatch = new CountDownLatch(1); + int windowSize = 1536; + Exchanger exchanger = new Exchanger<>(); + CountDownLatch settingsLatch = new CountDownLatch(1); + CountDownLatch dataLatch = new CountDownLatch(1); start(new ServerSessionListener.Adapter() { @Override @@ -429,7 +429,7 @@ public abstract class FlowControlStrategyTest stream.headers(responseFrame, Callback.NOOP); return new Stream.Listener.Adapter() { - private AtomicInteger dataFrames = new AtomicInteger(); + private final AtomicInteger dataFrames = new AtomicInteger(); @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -481,7 +481,7 @@ public abstract class FlowControlStrategyTest session.newStream(requestFrame, streamPromise, null); Stream stream = streamPromise.get(5, TimeUnit.SECONDS); - final int length = 5 * windowSize; + int length = 5 * windowSize; DataFrame dataFrame = new DataFrame(stream.getId(), ByteBuffer.allocate(length), true); stream.data(dataFrame, Callback.NOOP); @@ -500,7 +500,7 @@ public abstract class FlowControlStrategyTest assertTrue(dataLatch.await(5, TimeUnit.SECONDS)); } - private void checkThatWeAreFlowControlStalled(Exchanger exchanger) throws Exception + private void checkThatWeAreFlowControlStalled(Exchanger exchanger) { assertThrows(TimeoutException.class, () -> exchanger.exchange(null, 1, TimeUnit.SECONDS)); @@ -509,7 +509,7 @@ public abstract class FlowControlStrategyTest @Test public void testSessionStalledStallsNewStreams() throws Exception { - final int windowSize = 1024; + int windowSize = 1024; start(new ServerSessionListener.Adapter() { @Override @@ -544,8 +544,8 @@ public abstract class FlowControlStrategyTest Session session = newClient(new Session.Listener.Adapter()); // First request is just to consume most of the session window. - final List callbacks1 = new ArrayList<>(); - final CountDownLatch prepareLatch = new CountDownLatch(1); + List callbacks1 = new ArrayList<>(); + CountDownLatch prepareLatch = new CountDownLatch(1); MetaData.Request request1 = newRequest("POST", new HttpFields()); session.newStream(new HeadersFrame(request1, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @@ -584,7 +584,7 @@ public abstract class FlowControlStrategyTest }); // Fourth request is now stalled. - final CountDownLatch latch = new CountDownLatch(1); + CountDownLatch latch = new CountDownLatch(1); MetaData.Request request4 = newRequest("GET", new HttpFields()); session.newStream(new HeadersFrame(request4, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @@ -613,7 +613,7 @@ public abstract class FlowControlStrategyTest @Test public void testServerSendsBigContent() throws Exception { - final byte[] data = new byte[1024 * 1024]; + byte[] data = new byte[1024 * 1024]; new Random().nextBytes(data); start(new ServerSessionListener.Adapter() @@ -637,8 +637,8 @@ public abstract class FlowControlStrategyTest Session session = newClient(new Session.Listener.Adapter()); MetaData.Request metaData = newRequest("GET", new HttpFields()); HeadersFrame requestFrame = new HeadersFrame(metaData, null, true); - final byte[] bytes = new byte[data.length]; - final CountDownLatch latch = new CountDownLatch(1); + byte[] bytes = new byte[data.length]; + CountDownLatch latch = new CountDownLatch(1); session.newStream(requestFrame, new Promise.Adapter<>(), new Stream.Listener.Adapter() { private int received; @@ -682,7 +682,7 @@ public abstract class FlowControlStrategyTest } }); - final int initialWindow = 16; + int initialWindow = 16; Session session = newClient(new Session.Listener.Adapter() { @Override @@ -698,11 +698,11 @@ public abstract class FlowControlStrategyTest new Random().nextBytes(requestData); byte[] responseData = new byte[requestData.length]; - final ByteBuffer responseContent = ByteBuffer.wrap(responseData); + ByteBuffer responseContent = ByteBuffer.wrap(responseData); MetaData.Request metaData = newRequest("GET", new HttpFields()); HeadersFrame requestFrame = new HeadersFrame(metaData, null, false); Promise.Completable completable = new Promise.Completable<>(); - final CountDownLatch latch = new CountDownLatch(1); + CountDownLatch latch = new CountDownLatch(1); session.newStream(requestFrame, completable, new Stream.Listener.Adapter() { @Override @@ -747,7 +747,7 @@ public abstract class FlowControlStrategyTest } }); - final CountDownLatch closeLatch = new CountDownLatch(1); + CountDownLatch closeLatch = new CountDownLatch(1); Session session = newClient(new Session.Listener.Adapter() { @Override @@ -765,7 +765,7 @@ public abstract class FlowControlStrategyTest session.newStream(requestFrame, Promise.from(completable), new Stream.Listener.Adapter()); Stream stream = completable.get(5, TimeUnit.SECONDS); ByteBuffer data = ByteBuffer.allocate(FlowControlStrategy.DEFAULT_WINDOW_SIZE); - final CountDownLatch dataLatch = new CountDownLatch(1); + CountDownLatch dataLatch = new CountDownLatch(1); stream.data(new DataFrame(stream.getId(), data, false), new Callback() { @Override @@ -797,7 +797,7 @@ public abstract class FlowControlStrategyTest ByteBuffer extraData = ByteBuffer.allocate(1024); http2Session.getGenerator().data(lease, new DataFrame(stream.getId(), extraData, true), extraData.remaining()); List buffers = lease.getByteBuffers(); - http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[buffers.size()])); + http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[0])); // Expect the connection to be closed. assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); @@ -831,7 +831,7 @@ public abstract class FlowControlStrategyTest } }); - final CountDownLatch closeLatch = new CountDownLatch(1); + CountDownLatch closeLatch = new CountDownLatch(1); Session session = newClient(new Session.Listener.Adapter() { @Override @@ -849,7 +849,7 @@ public abstract class FlowControlStrategyTest session.newStream(requestFrame, streamPromise, new Stream.Listener.Adapter()); Stream stream = streamPromise.get(5, TimeUnit.SECONDS); ByteBuffer data = ByteBuffer.allocate(FlowControlStrategy.DEFAULT_WINDOW_SIZE); - final CountDownLatch dataLatch = new CountDownLatch(1); + CountDownLatch dataLatch = new CountDownLatch(1); stream.data(new DataFrame(stream.getId(), data, false), new Callback() { @Override @@ -877,7 +877,7 @@ public abstract class FlowControlStrategyTest ByteBuffer extraData = ByteBuffer.allocate(1024); http2Session.getGenerator().data(lease, new DataFrame(stream.getId(), extraData, true), extraData.remaining()); List buffers = lease.getByteBuffers(); - http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[buffers.size()])); + http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[0])); // Expect the connection to be closed. assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); @@ -917,7 +917,7 @@ public abstract class FlowControlStrategyTest MetaData.Request metaData = newRequest("POST", new HttpFields()); HeadersFrame frame = new HeadersFrame(metaData, null, false); FuturePromise streamPromise = new FuturePromise<>(); - final CountDownLatch resetLatch = new CountDownLatch(1); + CountDownLatch resetLatch = new CountDownLatch(1); session.newStream(frame, streamPromise, new Stream.Listener.Adapter() { @Override @@ -930,7 +930,7 @@ public abstract class FlowControlStrategyTest // Perform a big upload that will stall the flow control windows. ByteBuffer data = ByteBuffer.allocate(5 * FlowControlStrategy.DEFAULT_WINDOW_SIZE); - final CountDownLatch dataLatch = new CountDownLatch(1); + CountDownLatch dataLatch = new CountDownLatch(1); stream.data(new DataFrame(stream.getId(), data, true), new Callback() { @Override From 226d616a8a257a711d354b229149207c6f655d2f Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 10 Nov 2020 10:07:19 +0100 Subject: [PATCH 05/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Reimplemented close/idle_timeout/stop/onGoAway/input_shutdown following more closely the specification. In particular, the semantic of sending a GOAWAY is now to: * stop creation of new both local and remote streams * record the last processed stream * continue processing streams that are pending This means that a GOAWAY is "graceful" in the sense that it allows for streams to be completed by applications. The semantic of stop() and idle timeout is harsher: for pending streams a RST_STREAM is sent to the other peer and they are failed locally. Added support for GOAWAY with 2^31-1 lastStreamId. Added support for a peer to send and receive multiple GOAWAY frames. Reviewed the stream creation/destruction mechanism so that when the last stream completes after a GOAWAY, proper actions can be run to tear down the connection. Signed-off-by: Simone Bordet --- .../client/HTTP2ClientConnectionFactory.java | 14 - .../jetty/http2/client/AsyncServletTest.java | 9 +- .../http2/client/FlowControlStrategyTest.java | 56 +- .../jetty/http2/client/GoAwayTest.java | 963 +++++++++++++ .../http2/client/SessionFailureTest.java | 4 +- .../jetty/http2/client/StreamCloseTest.java | 4 +- .../eclipse/jetty/http2/HTTP2Connection.java | 2 +- .../org/eclipse/jetty/http2/HTTP2Flusher.java | 4 +- .../org/eclipse/jetty/http2/HTTP2Session.java | 1266 +++++++++++------ .../org/eclipse/jetty/http2/HTTP2Stream.java | 18 +- .../org/eclipse/jetty/http2/ISession.java | 3 +- .../org/eclipse/jetty/http2/api/Session.java | 12 +- .../jetty/http2/frames/GoAwayFrame.java | 24 +- .../jetty/http2/client/http/AbstractTest.java | 8 +- .../HttpClientTransportOverHTTP2Test.java | 4 +- .../RawHTTP2ServerConnectionFactory.java | 6 + .../eclipse/jetty/util/IteratingCallback.java | 2 +- 17 files changed, 1882 insertions(+), 517 deletions(-) create mode 100644 jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java index 870788cb399..388599be9b1 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java @@ -92,20 +92,6 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory this.listener = listener; } - @Override - public long getMessagesIn() - { - HTTP2ClientSession session = (HTTP2ClientSession)getSession(); - return session.getStreamsOpened(); - } - - @Override - public long getMessagesOut() - { - HTTP2ClientSession session = (HTTP2ClientSession)getSession(); - return session.getStreamsClosed(); - } - @Override public void onOpen() { diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AsyncServletTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AsyncServletTest.java index 7c971a452d1..140b2862a54 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AsyncServletTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AsyncServletTest.java @@ -136,7 +136,7 @@ public class AsyncServletTest extends AbstractTest HeadersFrame frame = new HeadersFrame(metaData, null, true); FuturePromise promise = new FuturePromise<>(); CountDownLatch responseLatch = new CountDownLatch(1); - CountDownLatch resetLatch = new CountDownLatch(1); + CountDownLatch failLatch = new CountDownLatch(1); session.newStream(frame, promise, new Stream.Listener.Adapter() { @Override @@ -146,9 +146,10 @@ public class AsyncServletTest extends AbstractTest } @Override - public void onReset(Stream stream, ResetFrame frame) + public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) { - resetLatch.countDown(); + failLatch.countDown(); + callback.succeeded(); } }); Stream stream = promise.get(5, TimeUnit.SECONDS); @@ -156,7 +157,7 @@ public class AsyncServletTest extends AbstractTest assertTrue(serverLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); assertFalse(responseLatch.await(idleTimeout + 1000, TimeUnit.MILLISECONDS)); - assertTrue(resetLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + assertTrue(failLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); } @Test diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java index 5d7a156e11d..3100f66078f 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlStrategyTest.java @@ -731,6 +731,7 @@ public abstract class FlowControlStrategyTest public void testClientExceedingSessionWindow() throws Exception { // On server, we don't consume the data. + CountDownLatch serverCloseLatch = new CountDownLatch(1); start(new ServerSessionListener.Adapter() { @Override @@ -745,16 +746,29 @@ public abstract class FlowControlStrategyTest } }; } - }); - CountDownLatch closeLatch = new CountDownLatch(1); - Session session = newClient(new Session.Listener.Adapter() - { @Override public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session session = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) { if (frame.getError() == ErrorCode.FLOW_CONTROL_ERROR.code) - closeLatch.countDown(); + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); } }); @@ -800,13 +814,16 @@ public abstract class FlowControlStrategyTest http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[0])); // Expect the connection to be closed. - assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); + assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); } @Test public void testClientExceedingStreamWindow() throws Exception { // On server, we don't consume the data. + CountDownLatch serverCloseLatch = new CountDownLatch(1); start(new ServerSessionListener.Adapter() { @Override @@ -829,16 +846,29 @@ public abstract class FlowControlStrategyTest } }; } - }); - CountDownLatch closeLatch = new CountDownLatch(1); - Session session = newClient(new Session.Listener.Adapter() - { @Override public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session session = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) { if (frame.getError() == ErrorCode.FLOW_CONTROL_ERROR.code) - closeLatch.countDown(); + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); } }); @@ -880,7 +910,9 @@ public abstract class FlowControlStrategyTest http2Session.getEndPoint().write(Callback.NOOP, buffers.toArray(new ByteBuffer[0])); // Expect the connection to be closed. - assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); + assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); } @Test diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java new file mode 100644 index 00000000000..a2c2345d6e2 --- /dev/null +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java @@ -0,0 +1,963 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http2.client; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.http.HttpVersion; +import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.CloseState; +import org.eclipse.jetty.http2.ErrorCode; +import org.eclipse.jetty.http2.HTTP2Session; +import org.eclipse.jetty.http2.api.Session; +import org.eclipse.jetty.http2.api.Stream; +import org.eclipse.jetty.http2.api.server.ServerSessionListener; +import org.eclipse.jetty.http2.frames.DataFrame; +import org.eclipse.jetty.http2.frames.GoAwayFrame; +import org.eclipse.jetty.http2.frames.HeadersFrame; +import org.eclipse.jetty.http2.frames.ResetFrame; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.FuturePromise; +import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class GoAwayTest extends AbstractTest +{ + @Test + public void testClientGoAwayServerReplies() throws Exception + { + CountDownLatch serverLatch = new CountDownLatch(1); + AtomicReference serverSessionRef = new AtomicReference<>(); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverSessionRef.set(stream.getSession()); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + stream.headers(new HeadersFrame(stream.getId(), response, null, true), Callback.NOOP); + return null; + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverLatch.countDown(); + } + }); + + CountDownLatch clientLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientLatch.countDown(); + } + }); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (frame.isEndStream() && response.getStatus() == HttpStatus.OK_200) + clientSession.close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + } + }); + + Assertions.assertTrue(serverLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertSame(CloseState.CLOSED, ((HTTP2Session)serverSessionRef.get()).getCloseState()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + Assertions.assertSame(CloseState.CLOSED, ((HTTP2Session)clientSession).getCloseState()); + } + + @Test + public void testServerGoAwayWithInFlightStreamClientFailsStream() throws Exception + { + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverSessionRef.set(stream.getSession()); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + stream.headers(new HeadersFrame(stream.getId(), response, null, true), Callback.NOOP); + return null; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + serverGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + + MetaData.Request request1 = newRequest(HttpMethod.GET.asString(), new HttpFields()); + CountDownLatch streamFailureLatch = new CountDownLatch(1); + clientSession.newStream(new HeadersFrame(request1, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + // Simulate the server closing while the client sends a second request. + // The server sends a lastStreamId for the first request, and discards the second. + serverSessionRef.get().close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + // The client sends the second request and should eventually fail it + // locally since it has a larger streamId, and the server discarded it. + MetaData.Request request2 = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request2, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) + { + streamFailureLatch.countDown(); + callback.succeeded(); + } + }); + } + }); + + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); +// Assertions.assertTrue(streamFailureLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + } + + @Test + public void testServerGracefulGoAway() throws Exception + { + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + AtomicReference serverSessionRef = new AtomicReference<>(); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverSessionRef.set(stream.getSession()); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + stream.headers(new HeadersFrame(stream.getId(), response, null, true), Callback.NOOP); + return null; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + serverGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + if (!frame.isGraceful()) + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGracefulGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + clientGracefulGoAwayLatch.countDown(); + else + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + if (!frame.isGraceful()) + clientCloseLatch.countDown(); + } + }); + CountDownLatch clientLatch = new CountDownLatch(1); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (frame.isEndStream() && response.getStatus() == HttpStatus.OK_200) + clientLatch.countDown(); + } + }); + + Assertions.assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); + + // Send a graceful GOAWAY from the server. + // Because the server had no pending streams, it will send also a non-graceful GOAWAY. + ((HTTP2Session)serverSessionRef.get()).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + Assertions.assertTrue(clientGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testServerGracefulGoAwayWithStreamsServerClosesWhenLastStreamCloses() throws Exception + { + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + AtomicReference serverSessionRef = new AtomicReference<>(); + AtomicReference serverStreamRef = new AtomicReference<>(); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverStreamRef.set(stream); + Session session = stream.getSession(); + serverSessionRef.set(session); + + // Send a graceful GOAWAY while processing a stream. + ((HTTP2Session)session).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + return null; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + serverGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + if (!frame.isGraceful()) + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGracefulGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + clientGracefulGoAwayLatch.countDown(); + else + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + if (!frame.isGraceful()) + clientCloseLatch.countDown(); + } + }); + CountDownLatch clientLatch = new CountDownLatch(1); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (frame.isEndStream() && response.getStatus() == HttpStatus.OK_200) + clientLatch.countDown(); + } + }); + + // Wait for the graceful GOAWAY. + Assertions.assertTrue(clientGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + + // Now the client cannot create new streams. + FuturePromise streamPromise = new FuturePromise<>(); + clientSession.newStream(new HeadersFrame(newRequest(HttpMethod.GET.asString(), new HttpFields()), null, true), streamPromise, null); + Assertions.assertThrows(ExecutionException.class, () -> streamPromise.get(5, TimeUnit.SECONDS)); + + // The client must not reply to a graceful GOAWAY. + Assertions.assertFalse(serverGoAwayLatch.await(1, TimeUnit.SECONDS)); + + // Previous streams must complete successfully. + Stream serverStream = serverStreamRef.get(); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + serverStream.headers(new HeadersFrame(serverStream.getId(), response, null, true), Callback.NOOP); + + Assertions.assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); + + // The server should have sent the GOAWAY after the last stream completed. + + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testClientGoAwayWithStreamsServerClosesWhenLastStreamCloses() throws Exception + { + AtomicReference serverStreamRef = new AtomicReference<>(); + CountDownLatch serverStreamLatch = new CountDownLatch(1); + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverStreamRef.set(stream); + serverStreamLatch.countDown(); + return null; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + serverGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + + CountDownLatch clientLatch = new CountDownLatch(1); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (frame.isEndStream() && response.getStatus() == HttpStatus.OK_200) + clientLatch.countDown(); + } + }); + + Assertions.assertTrue(serverStreamLatch.await(5, TimeUnit.SECONDS)); + + // The client sends a GOAWAY. + clientSession.close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); + + // The client must not receive a GOAWAY until the all streams are completed. + Assertions.assertFalse(clientGoAwayLatch.await(1, TimeUnit.SECONDS)); + + // Complete the stream. + Stream serverStream = serverStreamRef.get(); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + serverStream.headers(new HeadersFrame(serverStream.getId(), response, null, true), Callback.NOOP); + + Assertions.assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverStream.getSession()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testServerGracefulGoAwayWithStreamsClientGoAwayServerClosesWhenLastStreamCloses() throws Exception + { + AtomicReference serverStreamRef = new AtomicReference<>(); + CountDownLatch serverStreamLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverStreamRef.set(stream); + serverStreamLatch.countDown(); + + // Send a graceful GOAWAY while processing a stream. + ((HTTP2Session)stream.getSession()).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + return null; + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + { + // Send a GOAWAY when receiving a graceful GOAWAY. + session.close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + } + else + { + clientGoAwayLatch.countDown(); + } + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + + CountDownLatch clientLatch = new CountDownLatch(1); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + clientSession.newStream(new HeadersFrame(request, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (frame.isEndStream() && response.getStatus() == HttpStatus.OK_200) + clientLatch.countDown(); + } + }); + + // The server has a pending stream, so it does not send the non-graceful GOAWAY yet. + Assertions.assertFalse(clientGoAwayLatch.await(1, TimeUnit.SECONDS)); + + // Complete the stream, the server should send the non-graceful GOAWAY. + Stream serverStream = serverStreamRef.get(); + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + serverStream.headers(new HeadersFrame(serverStream.getId(), response, null, true), Callback.NOOP); + + // The server already received the client GOAWAY, + // so completing the last stream produces a close event. + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); + // The client should receive the server non-graceful GOAWAY. + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverStream.getSession()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testClientGracefulGoAwayWithStreamsServerGracefulGoAwayServerClosesWhenLastStreamCloses() throws Exception + { + AtomicReference serverStreamRef = new AtomicReference<>(); + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverStreamRef.set(stream); + return new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame frame, Callback callback) + { + if (frame.isEndStream()) + { + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + stream.headers(new HeadersFrame(stream.getId(), response, null, true), callback); + } + else + { + callback.succeeded(); + } + } + }; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + { + // Send a graceful GOAWAY. + ((HTTP2Session)session).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + } + else + { + serverGoAwayLatch.countDown(); + } + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGracefulGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + clientGracefulGoAwayLatch.countDown(); + else + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + FuturePromise promise = new FuturePromise<>(); + clientSession.newStream(new HeadersFrame(request, null, false), promise, new Stream.Listener.Adapter()); + Stream clientStream = promise.get(5, TimeUnit.SECONDS); + + // Send a graceful GOAWAY from the client. + ((HTTP2Session)clientSession).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + // The server should send a graceful GOAWAY. + Assertions.assertTrue(clientGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + + // Complete the stream. + clientStream.data(new DataFrame(clientStream.getId(), BufferUtil.EMPTY_BUFFER, true), Callback.NOOP); + + // Both client and server should send a non-graceful GOAWAY. + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverStreamRef.get().getSession()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testClientShutdownServerCloses() throws Exception + { + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverSessionRef.set(session); + serverCloseLatch.countDown(); + } + }); + + Session clientSession = newClient(new Session.Listener.Adapter()); + // Wait for the SETTINGS frames to be exchanged. + Thread.sleep(500); + + ((HTTP2Session)clientSession).getEndPoint().close(); + + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + } + + @Test + public void testServerGracefulGoAwayClientShutdownServerCloses() throws Exception + { + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onAccept(Session session) + { + serverSessionRef.set(session); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + // Reply to the graceful GOAWAY from the server with a TCP close. + ((HTTP2Session)session).getEndPoint().close(); + } + }); + // Wait for the SETTINGS frames to be exchanged. + Thread.sleep(500); + + // Send a graceful GOAWAY to the client. + ((HTTP2Session)serverSessionRef.get()).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + } + + @Test + public void testServerIdleTimeout() throws Exception + { + long idleTimeout = 1000; + + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverIdleTimeoutLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onAccept(Session session) + { + serverSessionRef.set(session); + ((HTTP2Session)session).getEndPoint().setIdleTimeout(idleTimeout); + } + + @Override + public boolean onIdleTimeout(Session session) + { + serverIdleTimeoutLatch.countDown(); + return true; + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + + Assertions.assertTrue(serverIdleTimeoutLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + // Server should send a GOAWAY to the client. + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + // The client replied to server's GOAWAY, but the server already closed. + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testServerGracefulGoAwayWithStreamsServerIdleTimeout() throws Exception + { + long idleTimeout = 1000; + + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onAccept(Session session) + { + serverSessionRef.set(session); + ((HTTP2Session)session).getEndPoint().setIdleTimeout(idleTimeout); + } + + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + stream.setIdleTimeout(10 * idleTimeout); + // Send a graceful GOAWAY. + ((HTTP2Session)stream.getSession()).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + return null; + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGracefulGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + clientGracefulGoAwayLatch.countDown(); + else + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + // Send request headers but not data. + clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter()); + + Assertions.assertTrue(clientGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + // Server idle timeout sends a non-graceful GOAWAY. + Assertions.assertTrue(clientGoAwayLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testClientGracefulGoAwayWithStreamsServerIdleTimeout() throws Exception + { + long idleTimeout = 1000; + + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverGracefulGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onAccept(Session session) + { + serverSessionRef.set(session); + ((HTTP2Session)session).getEndPoint().setIdleTimeout(idleTimeout); + } + + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + stream.setIdleTimeout(10 * idleTimeout); + return null; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + if (frame.isGraceful()) + serverGracefulGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + CountDownLatch streamFailureLatch = new CountDownLatch(1); + clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) + { + streamFailureLatch.countDown(); + callback.succeeded(); + } + }); + + // Client sends a graceful GOAWAY. + ((HTTP2Session)clientSession).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); + + Assertions.assertTrue(serverGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientGoAwayLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assertions.assertTrue(streamFailureLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testServerGoAwayWithStreamsThenStop() throws Exception + { + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + serverSessionRef.set(stream.getSession()); + // Don't reply, don't reset the stream, just send the GOAWAY. + stream.getSession().close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + return null; + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + + MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); + CountDownLatch clientResetLatch = new CountDownLatch(1); + clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onReset(Stream stream, ResetFrame frame) + { + clientResetLatch.countDown(); + } + }); + + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + + // Neither the client nor the server are finishing + // the pending stream, so force the stop on the server. + LifeCycle.stop(serverSessionRef.get()); + + // The server should reset all the pending streams. + Assertions.assertTrue(clientResetLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } +} diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/SessionFailureTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/SessionFailureTest.java index 5094e5846af..bf3e886ff39 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/SessionFailureTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/SessionFailureTest.java @@ -84,8 +84,8 @@ public class SessionFailureTest extends AbstractTest @Override public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) { - // Forcibly close the connection. - ((HTTP2Session)stream.getSession()).getEndPoint().close(); + // Forcibly shutdown the output to fail the write below. + ((HTTP2Session)stream.getSession()).getEndPoint().shutdownOutput(); // Now try to write something: it should fail. stream.headers(frame, new Callback() { diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/StreamCloseTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/StreamCloseTest.java index c2ecfc78bd2..a955cbcda54 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/StreamCloseTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/StreamCloseTest.java @@ -321,7 +321,9 @@ public class StreamCloseTest extends AbstractTest MetaData.Request request = (MetaData.Request)frame.getMetaData(); if ("GET".equals(request.getMethod())) { - ((HTTP2Session)stream.getSession()).getEndPoint().close(); + // Only shutdown the output, since closing the EndPoint causes a call to + // stop() on different thread which tries to concurrently fail the stream. + ((HTTP2Session)stream.getSession()).getEndPoint().shutdownOutput(); // Try to write something to force an error. stream.data(new DataFrame(stream.getId(), ByteBuffer.allocate(1024), true), Callback.NOOP); } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java index 8d3adb4e647..c37ffa0d222 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java @@ -239,7 +239,7 @@ public class HTTP2Connection extends AbstractConnection implements WriteFlusher. { Runnable task = pollTask(); if (LOG.isDebugEnabled()) - LOG.debug("Dequeued task {}", task); + LOG.debug("Dequeued task {}", String.valueOf(task)); if (task != null) return task; diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java index b228e4a340a..26efd12415b 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java @@ -365,7 +365,7 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable // If the failure came from within the // flusher, we need to close the connection. if (closed == null) - session.abort(x); + session.onWriteFailure(x); } void terminate(Throwable cause) @@ -376,7 +376,7 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable closed = terminated; terminated = cause; if (LOG.isDebugEnabled()) - LOG.debug("{}", closed != null ? "Terminated" : "Terminating"); + LOG.debug("{} {}", closed != null ? "Terminated" : "Terminating", this); } if (closed == null) iterate(); 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 6c038817946..01f9eaefe85 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 @@ -23,7 +23,6 @@ import java.nio.channels.ClosedChannelException; import java.nio.charset.StandardCharsets; import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -35,13 +34,13 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; import org.eclipse.jetty.http2.frames.DataFrame; -import org.eclipse.jetty.http2.frames.DisconnectFrame; import org.eclipse.jetty.http2.frames.FailureFrame; import org.eclipse.jetty.http2.frames.Frame; import org.eclipse.jetty.http2.frames.FrameType; @@ -72,6 +71,7 @@ import org.eclipse.jetty.util.component.ContainerLifeCycle; import org.eclipse.jetty.util.component.DumpableCollection; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; +import org.eclipse.jetty.util.thread.Locker; import org.eclipse.jetty.util.thread.Scheduler; @ManagedObject @@ -82,14 +82,13 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private final ConcurrentMap streams = new ConcurrentHashMap<>(); private final AtomicLong streamsOpened = new AtomicLong(); private final AtomicLong streamsClosed = new AtomicLong(); - private final StreamCreator streamCreator = new StreamCreator(); + private final StreamsState streamsState = new StreamsState(); private final AtomicInteger localStreamIds = new AtomicInteger(); private final AtomicInteger lastRemoteStreamId = new AtomicInteger(); private final AtomicInteger localStreamCount = new AtomicInteger(); private final AtomicBiInteger remoteStreamCount = new AtomicBiInteger(); private final AtomicInteger sendWindow = new AtomicInteger(); private final AtomicInteger recvWindow = new AtomicInteger(); - private final AtomicReference closed = new AtomicReference<>(CloseState.NOT_CLOSED); private final AtomicLong bytesWritten = new AtomicLong(); private final Scheduler scheduler; private final EndPoint endPoint; @@ -103,8 +102,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private int initialSessionRecvWindow; private int writeThreshold; private boolean pushEnabled; - private long idleTime; - private GoAwayFrame closeFrame; public HTTP2Session(Scheduler scheduler, EndPoint endPoint, Generator generator, Session.Listener listener, FlowControlStrategy flowControl, int initialStreamId) { @@ -117,13 +114,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio this.maxLocalStreams = -1; this.maxRemoteStreams = -1; this.localStreamIds.set(initialStreamId); - this.lastRemoteStreamId.set(isClientStream(initialStreamId) ? 0 : -1); this.streamIdleTimeout = endPoint.getIdleTimeout(); this.sendWindow.set(FlowControlStrategy.DEFAULT_WINDOW_SIZE); this.recvWindow.set(FlowControlStrategy.DEFAULT_WINDOW_SIZE); this.writeThreshold = 32 * 1024; this.pushEnabled = true; // SPEC: by default, push is enabled. - this.idleTime = System.nanoTime(); addBean(flowControl); addBean(flusher); } @@ -132,26 +127,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio protected void doStop() throws Exception { super.doStop(); - close(ErrorCode.NO_ERROR.code, "stop", new Callback() - { - @Override - public void succeeded() - { - disconnect(); - } - - @Override - public void failed(Throwable x) - { - disconnect(); - } - - @Override - public InvocationType getInvocationType() - { - return InvocationType.NON_BLOCKING; - } - }); + streamsState.halt("stop"); } @ManagedAttribute(value = "The flow control strategy", readonly = true) @@ -250,7 +226,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } @Override - public void onData(final DataFrame frame, Callback callback) + public void onData(DataFrame frame, Callback callback) { if (LOG.isDebugEnabled()) LOG.debug("Received {} on {}", frame, this); @@ -266,9 +242,22 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (stream != null) { if (getRecvWindow() < 0) - onConnectionFailure(ErrorCode.FLOW_CONTROL_ERROR.code, "session_window_exceeded", callback); + { + onSessionFailure(ErrorCode.FLOW_CONTROL_ERROR.code, "session_window_exceeded", callback); + } else - stream.process(frame, new DataCallback(callback, stream, flowControlLength)); + { + if (stream.updateRecvWindow(0) < 0) + { + // It's a bad client, it does not deserve to be + // treated gently by just resetting the stream. + onSessionFailure(ErrorCode.FLOW_CONTROL_ERROR.code, "stream_window_exceeded", callback); + } + else + { + stream.process(frame, new DataCallback(callback, stream, flowControlLength)); + } + } } else { @@ -280,7 +269,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (isStreamClosed(streamId)) reset(null, new ResetFrame(streamId, ErrorCode.STREAM_CLOSED_ERROR.code), callback); else - onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "unexpected_data_frame", callback); + onSessionFailure(ErrorCode.PROTOCOL_ERROR.code, "unexpected_data_frame", callback); } } @@ -455,38 +444,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * @see #close(int, String, Callback) * @see #onShutdown() * @see #onIdleTimeout() + * TODO: Review javadocs */ @Override - public void onGoAway(final GoAwayFrame frame) + public void onGoAway(GoAwayFrame frame) { if (LOG.isDebugEnabled()) LOG.debug("Received {} on {}", frame, this); - - while (true) - { - CloseState current = closed.get(); - switch (current) - { - case NOT_CLOSED: - { - if (closed.compareAndSet(current, CloseState.REMOTELY_CLOSED)) - { - // We received a GO_AWAY, so try to write - // what's in the queue and then disconnect. - closeFrame = frame; - onClose(frame, new DisconnectCallback()); - return; - } - break; - } - default: - { - if (LOG.isDebugEnabled()) - LOG.debug("Ignored {}, already closed", frame); - return; - } - } - } + streamsState.onGoAway(frame); } @Override @@ -532,18 +497,13 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public void onStreamFailure(int streamId, int error, String reason) { - Callback callback = new ResetCallback(streamId, error, Callback.NOOP); + Callback callback = Callback.from(() -> reset(getStream(streamId), new ResetFrame(streamId, error), Callback.NOOP)); Throwable failure = toFailure(error, reason); if (LOG.isDebugEnabled()) LOG.debug("Stream #{} failure {}", streamId, this, failure); - onStreamFailure(streamId, error, reason, failure, 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, failure), callback); + failStream(stream, error, reason, failure, callback); else callback.succeeded(); } @@ -551,22 +511,24 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public void onConnectionFailure(int error, String reason) { - onConnectionFailure(error, reason, Callback.NOOP); + onSessionFailure(error, reason, Callback.NOOP); } - protected void onConnectionFailure(int error, String reason, Callback callback) + private void onSessionFailure(int error, String reason, Callback callback) { - Throwable failure = toFailure(error, reason); - if (LOG.isDebugEnabled()) - LOG.debug("Session failure {}", this, failure); - onFailure(error, reason, failure, new CloseCallback(error, reason, callback)); + streamsState.onSessionFailure(error, reason, callback); } - protected void abort(Throwable failure) + void onWriteFailure(Throwable failure) + { + streamsState.onWriteFailure(failure); + } + + protected void abort(String reason, Throwable failure, Callback callback) { if (LOG.isDebugEnabled()) - LOG.debug("Session abort {}", this, failure); - onFailure(ErrorCode.NO_ERROR.code, null, failure, new TerminateCallback(failure)); + LOG.debug("Session abort {} for {}", reason, this, failure); + onFailure(ErrorCode.NO_ERROR.code, reason, failure, callback); } private void onFailure(int error, String reason, Throwable failure, Callback callback) @@ -576,26 +538,35 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio Callback countCallback = new CountingCallback(callback, count + 1); for (Stream stream : streams) { - onStreamFailure(stream.getId(), error, reason, failure, countCallback); + if (stream.isClosed()) + countCallback.succeeded(); + else + failStream(stream, error, reason, failure, countCallback); } notifyFailure(this, failure, countCallback); } - private void onClose(GoAwayFrame frame, Callback callback) + private void failStreams(Predicate matcher, String reason, boolean reset) { - int error = frame.getError(); - String reason = frame.tryConvertPayload(); + int error = ErrorCode.CANCEL_STREAM_ERROR.code; Throwable failure = toFailure(error, reason); - if (LOG.isDebugEnabled()) - LOG.debug("Session close {}", this, failure); - Collection streams = getStreams(); - int count = streams.size(); - Callback countCallback = new CountingCallback(callback, count + 1); - for (Stream stream : streams) + for (Stream stream : getStreams()) { - onStreamFailure(stream.getId(), error, reason, failure, countCallback); + if (stream.isClosed()) + continue; + if (!matcher.test((IStream)stream)) + continue; + if (LOG.isDebugEnabled()) + LOG.debug("Failing stream {} of {}", stream, this); + failStream(stream, error, reason, failure, Callback.NOOP); + if (reset) + stream.reset(new ResetFrame(stream.getId(), error), Callback.NOOP); } - notifyClose(this, frame, countCallback); + } + + private void failStream(Stream stream, int error, String reason, Throwable failure, Callback callback) + { + ((IStream)stream).process(new FailureFrame(error, reason, failure), callback); } private Throwable toFailure(int error, String reason) @@ -612,19 +583,19 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public void newStream(IStream.FrameList frames, Promise promise, Stream.Listener listener) { - streamCreator.newStream(frames, promise, listener); + streamsState.newLocalStream(frames, promise, listener); } @Override public int priority(PriorityFrame frame, Callback callback) { - return streamCreator.priority(frame, callback); + return streamsState.priority(frame, callback); } @Override public void push(IStream stream, Promise promise, PushPromiseFrame frame, Stream.Listener listener) { - streamCreator.push(frame, new Promise.Wrapper(promise) + streamsState.push(frame, new Promise.Wrapper(promise) { @Override public void succeeded(Stream pushedStream) @@ -686,37 +657,22 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * @see #onGoAway(GoAwayFrame) * @see #onShutdown() * @see #onIdleTimeout() + * // TODO: review javadocs */ @Override public boolean close(int error, String reason, Callback callback) { - while (true) - { - CloseState current = closed.get(); - switch (current) - { - case NOT_CLOSED: - { - if (closed.compareAndSet(current, CloseState.LOCALLY_CLOSED)) - { - closeFrame = newGoAwayFrame(CloseState.LOCALLY_CLOSED, error, reason); - control(null, callback, closeFrame); - return true; - } - break; - } - default: - { - if (LOG.isDebugEnabled()) - LOG.debug("Ignoring close {}/{}, already closed", error, reason); - callback.succeeded(); - return false; - } - } - } + if (LOG.isDebugEnabled()) + LOG.debug("Closing {}/{} {}", ErrorCode.toString(error, null), reason, this); + return goAway(newGoAwayFrame(error, reason), callback); } - private GoAwayFrame newGoAwayFrame(CloseState closeState, int error, String reason) + public boolean goAway(GoAwayFrame frame, Callback callback) + { + return streamsState.goAway(frame, callback); + } + + private GoAwayFrame newGoAwayFrame(int error, String reason) { byte[] payload = null; if (reason != null) @@ -725,13 +681,18 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio reason = reason.substring(0, Math.min(reason.length(), 32)); payload = reason.getBytes(StandardCharsets.UTF_8); } - return new GoAwayFrame(closeState, getLastRemoteStreamId(), error, payload); + return new GoAwayFrame(getLastRemoteStreamId(), error, payload); } @Override public boolean isClosed() { - return closed.get() != CloseState.NOT_CLOSED; + return getCloseState() != CloseState.NOT_CLOSED; + } + + public CloseState getCloseState() + { + return streamsState.getCloseState(); } private void control(IStream stream, Callback callback, Frame frame) @@ -786,15 +747,17 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } - protected IStream createLocalStream(int streamId) + protected IStream createLocalStream(int streamId, Promise promise) { while (true) { int localCount = localStreamCount.get(); int maxCount = getMaxLocalStreams(); if (maxCount >= 0 && localCount >= maxCount) - // TODO: remove the dump() in the exception message. - throw new IllegalStateException("Max local stream count " + maxCount + " exceeded" + System.lineSeparator() + dump()); + { + promise.failed(new IllegalStateException("Max local stream count " + maxCount + " exceeded")); + return null; + } if (localStreamCount.compareAndSet(localCount, localCount + 1)) break; } @@ -811,12 +774,23 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio else { localStreamCount.decrementAndGet(); - throw new IllegalStateException("Duplicate stream " + streamId); + promise.failed(new IllegalStateException("Duplicate stream " + streamId)); + return null; } } protected IStream createRemoteStream(int streamId) { + if (!streamsState.newRemoteStream(streamId)) + { + if (LOG.isDebugEnabled()) + LOG.debug("Could not create remote stream #{} for {}", streamId, this); + return null; + } + + // This stream has been seen the server. + updateLastRemoteStreamId(streamId); + // SPEC: exceeding max concurrent streams is treated as stream error. while (true) { @@ -826,8 +800,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio int maxCount = getMaxRemoteStreams(); if (maxCount >= 0 && remoteCount - remoteClosing >= maxCount) { - updateLastRemoteStreamId(streamId); - reset(null, new ResetFrame(streamId, ErrorCode.REFUSED_STREAM_ERROR.code), Callback.NOOP); + reset(null, new ResetFrame(streamId, ErrorCode.REFUSED_STREAM_ERROR.code), Callback.from(() -> onStreamDestroyed(streamId))); return null; } if (remoteStreamCount.compareAndSet(encoded, remoteCount + 1, remoteClosing)) @@ -835,11 +808,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } IStream stream = newStream(streamId, false); - - // SPEC: duplicate stream is treated as connection error. if (streams.putIfAbsent(streamId, stream) == null) { - updateLastRemoteStreamId(streamId); stream.setIdleTimeout(getStreamIdleTimeout()); flowControl.onStreamCreated(stream); if (LOG.isDebugEnabled()) @@ -849,6 +819,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio else { remoteStreamCount.addAndGetHi(-1); + onStreamDestroyed(streamId); + // SPEC: duplicate stream is treated as connection error. onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "duplicate_stream"); return null; } @@ -868,16 +840,18 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } @Override - public void removeStream(IStream stream) + public boolean removeStream(IStream stream) { - IStream removed = streams.remove(stream.getId()); - if (removed != null) - { - onStreamClosed(stream); - flowControl.onStreamDestroyed(stream); - if (LOG.isDebugEnabled()) - LOG.debug("Removed {} {} from {}", stream.isLocal() ? "local" : "remote", stream, this); - } + int streamId = stream.getId(); + IStream removed = streams.remove(streamId); + if (removed == null) + return false; + if (LOG.isDebugEnabled()) + LOG.debug("Removed {} {} from {}", stream.isLocal() ? "local" : "remote", stream, this); + onStreamClosed(stream); + flowControl.onStreamDestroyed(stream); + onStreamDestroyed(streamId); + return true; } @Override @@ -889,7 +863,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @ManagedAttribute("The number of active streams") public int getStreamCount() { - return streams.size(); + return streamsState.streamCount.intValue(); } @Override @@ -964,41 +938,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * @see #onGoAway(GoAwayFrame) * @see #close(int, String, Callback) * @see #onIdleTimeout() + * TODO: review javadocs */ @Override public void onShutdown() { - if (LOG.isDebugEnabled()) - LOG.debug("Shutting down {}", this); - - switch (closed.get()) - { - case NOT_CLOSED: - { - // The other peer did not send a GO_AWAY, no need to be gentle. - if (LOG.isDebugEnabled()) - LOG.debug("Abrupt close for {}", this); - abort(new ClosedChannelException()); - break; - } - case LOCALLY_CLOSED: - { - // We have closed locally, and only shutdown - // the output; now queue a disconnect. - control(null, Callback.NOOP, new DisconnectFrame()); - break; - } - case REMOTELY_CLOSED: - { - // Nothing to do, the GO_AWAY frame we - // received will close the connection. - break; - } - default: - { - break; - } - } + streamsState.onShutdown(this); } /** @@ -1020,35 +965,17 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * @see #onGoAway(GoAwayFrame) * @see #close(int, String, Callback) * @see #onShutdown() + * TODO: review javadocs */ @Override public boolean onIdleTimeout() { - switch (closed.get()) - { - case NOT_CLOSED: - { - long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - idleTime); - if (elapsed < endPoint.getIdleTimeout()) - return false; - return notifyIdleTimeout(this); - } - case LOCALLY_CLOSED: - case REMOTELY_CLOSED: - { - abort(new TimeoutException("Idle timeout " + endPoint.getIdleTimeout() + " ms")); - return false; - } - default: - { - return false; - } - } + return streamsState.onIdleTimeout(this); } private void notIdle() { - idleTime = System.nanoTime(); + streamsState.idleTime = System.nanoTime(); } @Override @@ -1057,22 +984,46 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "upgrade"); } + protected void onStreamCreated(int streamId) + { + if (LOG.isDebugEnabled()) + LOG.debug("Created stream #{} for {}", streamId, this); + streamsState.onStreamCreated(); + } + protected void onStreamOpened(IStream stream) { + if (LOG.isDebugEnabled()) + LOG.debug("Opened stream {} for {}", stream, this); streamsOpened.incrementAndGet(); } protected void onStreamClosed(IStream stream) { + if (LOG.isDebugEnabled()) + LOG.debug("Closed stream {} for {}", stream, this); streamsClosed.incrementAndGet(); } + protected void onStreamDestroyed(int streamId) + { + if (LOG.isDebugEnabled()) + LOG.debug("Destroyed stream #{} for {}", streamId, this); + streamsState.onStreamDestroyed(); + } + @Override public void onFlushed(long bytes) throws IOException { flusher.onFlushed(bytes); } + private void terminate(Throwable cause) + { + flusher.terminate(cause); + disconnect(); + } + public void disconnect() { if (LOG.isDebugEnabled()) @@ -1080,38 +1031,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio endPoint.close(); } - private void terminate(Throwable cause) - { - while (true) - { - CloseState current = closed.get(); - switch (current) - { - case NOT_CLOSED: - case LOCALLY_CLOSED: - case REMOTELY_CLOSED: - { - if (closed.compareAndSet(current, CloseState.CLOSED)) - { - flusher.terminate(cause); - for (IStream stream : streams.values()) - { - stream.close(); - } - streams.clear(); - disconnect(); - return; - } - break; - } - default: - { - return; - } - } - } - } - public boolean isDisconnected() { return !endPoint.isOpen(); @@ -1176,6 +1095,18 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } + protected void notifyGoAway(Session session, GoAwayFrame frame) + { + try + { + listener.onGoAway(session, frame); + } + catch (Throwable x) + { + LOG.info("Failure while notifying listener " + listener, x); + } + } + protected void notifyClose(Session session, GoAwayFrame frame, Callback callback) { try @@ -1243,16 +1174,15 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public String toString() { - return String.format("%s@%x{l:%s <-> r:%s,sendWindow=%s,recvWindow=%s,streams=%d,%s,%s}", + return String.format("%s@%x{local:%s,remote:%s,sendWindow=%s,recvWindow=%s,%s}", getClass().getSimpleName(), hashCode(), getEndPoint().getLocalAddress(), getEndPoint().getRemoteAddress(), sendWindow, recvWindow, - streams.size(), - closed, - closeFrame); + streamsState + ); } private class ControlEntry extends HTTP2Flusher.Entry @@ -1342,29 +1272,18 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { case HEADERS: { - onStreamOpened(stream); HeadersFrame headersFrame = (HeadersFrame)frame; + if (headersFrame.getMetaData().isRequest()) + onStreamOpened(stream); if (stream.updateClose(headersFrame.isEndStream(), CloseState.Event.AFTER_SEND)) removeStream(stream); break; } - case GO_AWAY: - { - // We just sent a GO_AWAY, only shutdown the - // output without closing yet, to allow reads. - getEndPoint().shutdownOutput(); - break; - } case WINDOW_UPDATE: { flowControl.windowUpdate(HTTP2Session.this, stream, (WindowUpdateFrame)frame); break; } - case DISCONNECT: - { - terminate(new ClosedChannelException()); - break; - } default: { break; @@ -1373,14 +1292,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio super.succeeded(); } - - @Override - public void failed(Throwable x) - { - if (frame.getType() == FrameType.DISCONNECT) - terminate(new ClosedChannelException()); - super.failed(x); - } } private class DataEntry extends HTTP2Flusher.Entry @@ -1483,30 +1394,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } - private static class StreamPromiseCallback implements Callback - { - private final Promise promise; - private final IStream stream; - - private StreamPromiseCallback(Promise promise, IStream stream) - { - this.promise = promise; - this.stream = stream; - } - - @Override - public void succeeded() - { - promise.succeeded(stream); - } - - @Override - public void failed(Throwable x) - { - promise.failed(x); - } - } - private class DataCallback extends Callback.Nested { private final IStream stream; @@ -1543,36 +1430,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } - private class ResetCallback extends Callback.Nested - { - private final int streamId; - private final int error; - - private ResetCallback(int streamId, int error, Callback callback) - { - super(callback); - this.streamId = streamId; - this.error = error; - } - - @Override - public void succeeded() - { - complete(); - } - - @Override - public void failed(Throwable x) - { - complete(); - } - - private void complete() - { - reset(getStream(streamId), new ResetFrame(streamId, error), getCallback()); - } - } - private class OnResetCallback implements Callback { @Override @@ -1599,97 +1456,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } - private class CloseCallback extends Callback.Nested - { - private final int error; - private final String reason; - - private CloseCallback(int error, String reason, Callback callback) - { - super(callback); - this.error = error; - this.reason = reason; - } - - @Override - public void succeeded() - { - complete(); - } - - @Override - public void failed(Throwable x) - { - complete(); - } - - private void complete() - { - close(error, reason, getCallback()); - } - } - - private class DisconnectCallback implements Callback - { - @Override - public void succeeded() - { - complete(); - } - - @Override - public void failed(Throwable x) - { - complete(); - } - - @Override - public InvocationType getInvocationType() - { - return InvocationType.NON_BLOCKING; - } - - private void complete() - { - frames(null, Arrays.asList(newGoAwayFrame(CloseState.CLOSED, ErrorCode.NO_ERROR.code, null), new DisconnectFrame()), Callback.NOOP); - } - } - - private class TerminateCallback implements Callback - { - private final Throwable failure; - - private TerminateCallback(Throwable failure) - { - this.failure = failure; - } - - @Override - public void succeeded() - { - complete(); - } - - @Override - public void failed(Throwable x) - { - if (x != failure) - failure.addSuppressed(x); - complete(); - } - - @Override - public InvocationType getInvocationType() - { - return InvocationType.NON_BLOCKING; - } - - private void complete() - { - terminate(failure); - } - } - /** * SPEC: It is required that stream ids are monotonically increasing. * Here we use a queue to atomically create the stream id and @@ -1697,107 +1463,696 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * flush up to the slot with a non-null entry to make sure * frames are sent strictly in their stream id order. * See https://tools.ietf.org/html/rfc7540#section-5.1.1. + * TODO: javadocs */ - private class StreamCreator + private class StreamsState { + private final Locker lock = new Locker(); private final Queue slots = new ArrayDeque<>(); + private final AtomicLong streamCount = new AtomicLong(); + private long idleTime = System.nanoTime(); + private CloseState closed = CloseState.NOT_CLOSED; + private Runnable closingAction; + private GoAwayFrame goAwayRecv; + private GoAwayFrame goAwaySent; + private Throwable failure; private Thread flushing; + private CloseState getCloseState() + { + try (Locker.Lock l = lock.lock()) + { + return closed; + } + } + + private boolean goAway(GoAwayFrame frame, Callback callback) + { + Runnable action = null; + boolean sendGoAway = false; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + { + goAwaySent = frame; + closed = CloseState.LOCALLY_CLOSED; + sendGoAway = true; + if (frame.isGraceful()) + { + // Try to send the non-graceful GOAWAY + // when the last stream is destroyed. + closingAction = action = () -> + { + // TODO: verify this + GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + goAway(goAwayFrame, Callback.from(Callback.NOOP::succeeded, HTTP2Session.this::terminate)); + }; + } + break; + } + case LOCALLY_CLOSED: + { + if (frame.isGraceful()) + { + // Trying to send a non-first, but graceful, GOAWAY, ignore this one. + if (LOG.isDebugEnabled()) + LOG.debug("Already sent, ignored GOAWAY {} for {}", frame, HTTP2Session.this); + } + else + { + if (goAwaySent.isGraceful()) + { + goAwaySent = frame; + sendGoAway = true; + } + else + { + // Trying to send another non-graceful GOAWAY, ignore this one. + if (LOG.isDebugEnabled()) + LOG.debug("Already sent, ignored GOAWAY {} for {}", frame, HTTP2Session.this); + } + } + break; + } + case REMOTELY_CLOSED: + { + goAwaySent = frame; + sendGoAway = true; + if (frame.isGraceful()) + { + // Try to send the non-graceful GOAWAY + // when the last stream is destroyed. + closingAction = action = () -> + { + // TODO: verify this + GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + goAway(goAwayFrame, Callback.from(Callback.NOOP::succeeded, HTTP2Session.this::terminate)); + }; + } + else + { + if (goAwayRecv.isGraceful()) + { + if (LOG.isDebugEnabled()) + LOG.debug("Waiting non-graceful GOAWAY for {}", HTTP2Session.this); + } + else + { + closed = CloseState.CLOSING; + closingAction = action = () -> terminate(frame); + } + } + break; + } + default: + { + // Already closing or closed, ignore it. + if (LOG.isDebugEnabled()) + LOG.debug("Already closed, ignored {} for {}", frame, HTTP2Session.this); + break; + } + } + } + + if (sendGoAway) + { + if (action == null) + sendGoAway(frame, callback); + else + sendGoAway(frame, Callback.from(callback, this::tryRunClosingAction)); + return true; + } + else + { + callback.succeeded(); + return false; + } + } + + private void halt(String reason) + { + if (LOG.isDebugEnabled()) + LOG.debug("Halting ({}) for {}", reason, HTTP2Session.this); + GoAwayFrame frame; + boolean sendGoAway = false; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + case REMOTELY_CLOSED: + case LOCALLY_CLOSED: + case CLOSING: + { + if (goAwaySent == null || goAwaySent.isGraceful()) + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + sendGoAway = true; + } + frame = goAwaySent; + closed = CloseState.CLOSED; + failure = toFailure(ErrorCode.NO_ERROR.code, reason); + break; + } + default: + { + return; + } + } + } + failStreams(stream -> true, reason, true); + if (sendGoAway) + sendGoAwayAndTerminate(frame); + else + terminate(frame); + } + + private void onGoAway(GoAwayFrame frame) + { + boolean failStreams = false; + Runnable action = null; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + { + goAwayRecv = frame; + closed = CloseState.REMOTELY_CLOSED; + if (frame.isGraceful()) + { + if (LOG.isDebugEnabled()) + LOG.debug("Waiting non-graceful GOAWAY for {}", HTTP2Session.this); + } + else + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + closed = CloseState.CLOSING; + closingAction = action = () -> sendGoAwayAndTerminate(frame); + failStreams = true; + } + break; + } + case LOCALLY_CLOSED: + { + goAwayRecv = frame; + if (frame.isGraceful()) + { + // Wait for the non-graceful GOAWAY from the other peer. + if (LOG.isDebugEnabled()) + LOG.debug("Waiting non-graceful GOAWAY for {}", HTTP2Session.this); + } + else + { + if (goAwaySent.isGraceful()) + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + closed = CloseState.CLOSING; + closingAction = action = () -> sendGoAwayAndTerminate(frame); + } + else + { + closed = CloseState.CLOSING; + closingAction = action = () -> terminate(frame); + failStreams = true; + } + } + break; + } + case REMOTELY_CLOSED: + { + if (frame.isGraceful()) + { + // Received a non-first, but graceful, GOAWAY, ignore it. + if (LOG.isDebugEnabled()) + LOG.debug("Already received, ignoring GOAWAY for {}", HTTP2Session.this); + } + else + { + boolean shouldSend = goAwaySent == null || goAwaySent.isGraceful(); + goAwayRecv = frame; + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + closed = CloseState.CLOSING; + closingAction = action = shouldSend ? () -> sendGoAwayAndTerminate(frame) : () -> terminate(frame); + failStreams = true; + } + break; + } + default: + { + // Already closing or closed, ignore it. + if (LOG.isDebugEnabled()) + LOG.debug("Already closed, ignored {} for {}", frame, HTTP2Session.this); + break; + } + } + } + + notifyGoAway(HTTP2Session.this, frame); + + if (failStreams) + { + // Must compare the lastStreamId only with local streams. + // For example, a client that sent request with streamId=137 may send a GOAWAY(4), + // where streamId=4 is the last stream pushed by the server to the client. + // The server must not compare its local streamId=4 with remote streamId=137. + Predicate failIf = stream -> stream.isLocal() && stream.getId() > frame.getLastStreamId(); + failStreams(failIf, "closing", false); + } + + if (action != null) + tryRunClosingAction(); + } + + private void onShutdown(HTTP2Session session) + { + String reason = "input_shutdown"; + boolean resetStreams = false; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + case LOCALLY_CLOSED: + { + if (LOG.isDebugEnabled()) + LOG.debug("Unexpected ISHUT for {}", session); + closed = CloseState.CLOSING; + GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); + closingAction = () -> terminate(frame); + failure = new ClosedChannelException(); + break; + } + case REMOTELY_CLOSED: + { + closed = CloseState.CLOSING; + GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); + closingAction = () -> terminate(frame); + failure = new ClosedChannelException(); + resetStreams = true; + break; + } + case CLOSING: + { + if (failure == null) + failure = new ClosedChannelException(); + resetStreams = true; + break; + } + default: + { + if (LOG.isDebugEnabled()) + LOG.debug("Already closed, ignoring ISHUT for {}", session); + return; + } + } + } + + if (resetStreams) + { + // Since nothing else will arrive from the other peer, reset + // the streams for which the other peer did not send all frames. + Predicate failIf = stream -> !stream.isRemotelyClosed(); + failStreams(failIf, reason, false); + tryRunClosingAction(); + } + else + { + abort(reason, failure, Callback.from(this::tryRunClosingAction)); + } + } + + private boolean onIdleTimeout(HTTP2Session session) + { + String reason = "idle_timeout"; + boolean notify = false; + Throwable cause = null; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + { + long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - idleTime); + if (elapsed < endPoint.getIdleTimeout()) + return false; + notify = true; + break; + } + // Timed out while waiting for closing events. + case LOCALLY_CLOSED: + { + boolean shouldSend = goAwaySent.isGraceful(); + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + closed = CloseState.CLOSING; + closingAction = shouldSend ? () -> sendGoAwayAndTerminate(goAwaySent) : () -> terminate(goAwaySent); + failure = cause = new TimeoutException("Session idle timeout expired"); + break; + } + case REMOTELY_CLOSED: + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + closed = CloseState.CLOSING; + closingAction = () -> sendGoAwayAndTerminate(goAwaySent); + failure = cause = new TimeoutException("Session idle timeout expired"); + break; + } + default: + { + if (LOG.isDebugEnabled()) + LOG.debug("Already closed, ignored idle timeout for {}", session); + return false; + } + } + } + + if (notify) + { + boolean close = notifyIdleTimeout(session); + if (LOG.isDebugEnabled()) + LOG.debug("Idle timeout {} for {}", close ? "confirmed" : "ignored", session); + if (close) + halt(reason); + return false; + } + + abort(reason, cause, Callback.from(this::tryRunClosingAction)); + return false; + } + + private void onSessionFailure(int error, String reason, Callback callback) + { + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + { + goAwayRecv = goAwaySent = newGoAwayFrame(error, reason); + closed = CloseState.CLOSING; + closingAction = () -> sendGoAwayAndTerminate(goAwaySent); + failure = toFailure(error, reason); + break; + } + case LOCALLY_CLOSED: + { + goAwayRecv = newGoAwayFrame(error, reason); + closed = CloseState.CLOSING; + closingAction = goAwaySent.isGraceful() ? () -> sendGoAwayAndTerminate(goAwayRecv) : () -> terminate(goAwayRecv); + failure = toFailure(error, reason); + break; + } + case REMOTELY_CLOSED: + { + goAwaySent = newGoAwayFrame(error, reason); + closed = CloseState.CLOSING; + closingAction = () -> sendGoAwayAndTerminate(goAwaySent); + failure = toFailure(error, reason); + break; + } + default: + { + if (LOG.isDebugEnabled()) + LOG.debug("Already closed, ignored session failure {}", HTTP2Session.this, failure); + callback.succeeded(); + return; + } + } + } + + if (LOG.isDebugEnabled()) + LOG.debug("Session failure {}", HTTP2Session.this, failure); + onFailure(error, reason, failure, Callback.from(callback, this::tryRunClosingAction)); + } + + private void onWriteFailure(Throwable x) + { + String reason = "write_failure"; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case NOT_CLOSED: + case LOCALLY_CLOSED: + case REMOTELY_CLOSED: + { + closed = CloseState.CLOSING; + closingAction = () -> terminate(newGoAwayFrame(ErrorCode.NO_ERROR.code, reason)); + failure = x; + break; + } + default: + { + return; + } + } + } + abort(reason, x, Callback.from(this::tryRunClosingAction)); + } + + private void sendGoAwayAndTerminate(GoAwayFrame frame) + { + sendGoAway(frame, Callback.from(() -> terminate(frame))); + } + + private void sendGoAway(GoAwayFrame frame, Callback callback) + { + control(null, callback, frame); + } + + private void onStreamCreated() + { + streamCount.incrementAndGet(); + } + + private void onStreamDestroyed() + { + long count = streamCount.decrementAndGet(); + if (count == 0) + runClosingAction(); + } + + private void tryRunClosingAction() + { + long count = streamCount.get(); + if (count == 0) + { + runClosingAction(); + } + else + { + if (LOG.isDebugEnabled()) + LOG.debug("Deferred closing action, {} pending streams on {}", count, HTTP2Session.this); + } + } + + private void runClosingAction() + { + // Threads from onStreamClosed() and other events + // such as onGoAway() may be in a race to finish, + // but only one moves to CLOSED and runs the action. + Runnable action = null; + try (Locker.Lock l = lock.lock()) + { + switch (closed) + { + case LOCALLY_CLOSED: + { + if (goAwaySent.isGraceful()) + { + action = closingAction; + closingAction = null; + } + break; + } + case REMOTELY_CLOSED: + { + if (goAwaySent != null && goAwaySent.isGraceful()) + { + action = closingAction; + closingAction = null; + } + break; + } + case CLOSING: + { + closed = CloseState.CLOSED; + action = closingAction; + closingAction = null; + break; + } + default: + { + break; + } + } + } + if (action != null) + { + if (LOG.isDebugEnabled()) + LOG.debug("Executing closing action on {}", HTTP2Session.this); + action.run(); + } + } + + private void terminate(GoAwayFrame frame) + { + if (LOG.isDebugEnabled()) + LOG.debug("Terminating {}", HTTP2Session.this); + HTTP2Session.this.terminate(failure); + notifyClose(HTTP2Session.this, frame, Callback.NOOP); + } + private int priority(PriorityFrame frame, Callback callback) { Slot slot = new Slot(); int currentStreamId = frame.getStreamId(); - int streamId = reserveSlot(slot, currentStreamId); - - if (currentStreamId <= 0) - frame = frame.withStreamId(streamId); - - slot.entries = Collections.singletonList(newEntry(frame, null, callback)); - flush(); + int streamId = reserveSlot(slot, currentStreamId, callback::failed); + if (streamId > 0) + { + if (currentStreamId <= 0) + frame = frame.withStreamId(streamId); + slot.entries = Collections.singletonList(newEntry(frame, null, Callback.from(callback::succeeded, x -> + { + HTTP2Session.this.onStreamDestroyed(streamId); + callback.failed(x); + }))); + flush(); + } return streamId; } - private void newStream(IStream.FrameList frameList, Promise promise, Stream.Listener listener) + private void newLocalStream(IStream.FrameList frameList, Promise promise, Stream.Listener listener) { Slot slot = new Slot(); int currentStreamId = frameList.getStreamId(); - int streamId = reserveSlot(slot, currentStreamId); - - List frames = frameList.getFrames(); - if (currentStreamId <= 0) + int streamId = reserveSlot(slot, currentStreamId, promise::failed); + if (streamId > 0) { - frames = frames.stream() - .map(frame -> frame.withStreamId(streamId)) - .collect(Collectors.toList()); + List frames = frameList.getFrames(); + if (currentStreamId <= 0) + { + frames = frames.stream() + .map(frame -> frame.withStreamId(streamId)) + .collect(Collectors.toList()); + } + if (createLocalStream(slot, frames, promise, listener, streamId)) + return; + freeSlot(slot, streamId); } + } - try + private boolean newRemoteStream(int streamId) + { + try (Locker.Lock l = lock.lock()) { - createLocalStream(slot, frames, promise, listener, streamId); - } - catch (Throwable x) - { - freeSlotAndFailPromise(slot, promise, x); + switch (closed) + { + case NOT_CLOSED: + { + HTTP2Session.this.onStreamCreated(streamId); + return true; + } + case LOCALLY_CLOSED: + { + // SPEC: streams larger than GOAWAY's lastStreamId are dropped. + if (streamId <= goAwaySent.getLastStreamId()) + { + // Allow creation of streams that may have been in-flight. + HTTP2Session.this.onStreamCreated(streamId); + return true; + } + return false; + } + default: + return false; + } } } private void push(PushPromiseFrame frame, Promise promise, Stream.Listener listener) { Slot slot = new Slot(); - int streamId = reserveSlot(slot, 0); - frame = frame.withStreamId(streamId); - - try + int streamId = reserveSlot(slot, 0, promise::failed); + if (streamId > 0) { - createLocalStream(slot, Collections.singletonList(frame), promise, listener, streamId); - } - catch (Throwable x) - { - freeSlotAndFailPromise(slot, promise, x); + frame = frame.withStreamId(streamId); + if (createLocalStream(slot, Collections.singletonList(frame), promise, listener, streamId)) + return; + freeSlot(slot, streamId); } } - private int reserveSlot(Slot slot, int streamId) + private boolean createLocalStream(Slot slot, List frames, Promise promise, Stream.Listener listener, int streamId) { - if (streamId <= 0) + IStream stream = HTTP2Session.this.createLocalStream(streamId, promise); + if (stream == null) + return false; + + stream.setListener(listener); + Callback streamCallback = Callback.from(() -> promise.succeeded(stream), x -> { - synchronized (this) - { - streamId = localStreamIds.getAndAdd(2); - slots.offer(slot); - } + HTTP2Session.this.onStreamDestroyed(streamId); + promise.failed(x); + }); + int count = frames.size(); + if (count == 1) + { + slot.entries = Collections.singletonList(newEntry(frames.get(0), stream, streamCallback)); } else { - synchronized (this) + Callback callback = new CountingCallback(streamCallback, count); + slot.entries = frames.stream() + .map(frame -> newEntry(frame, stream, callback)) + .collect(Collectors.toList()); + } + flush(); + return true; + } + + private int reserveSlot(Slot slot, int streamId, Consumer fail) + { + int newStreamId = streamId; + Throwable failure = null; + try (Locker.Lock l = lock.lock()) + { + if (closed == CloseState.NOT_CLOSED) { + if (newStreamId <= 0) + newStreamId = localStreamIds.getAndAdd(2); slots.offer(slot); } + else + { + failure = this.failure; + if (failure == null) + failure = new IllegalStateException("session closed"); + } + } + if (failure == null) + { + if (streamId <= 0) + HTTP2Session.this.onStreamCreated(newStreamId); + return newStreamId; + } + else + { + fail.accept(failure); + return 0; } - return streamId; } - private void createLocalStream(Slot slot, List frames, Promise promise, Stream.Listener listener, int streamId) + private void freeSlot(Slot slot, int streamId) { - IStream stream = HTTP2Session.this.createLocalStream(streamId); - stream.setListener(listener); - int count = frames.size(); - Callback streamCallback = new StreamPromiseCallback(promise, stream); - Callback callback = count == 1 ? streamCallback : new CountingCallback(streamCallback, count); - slot.entries = frames.stream() - .map(frame -> newEntry(frame, stream, callback)) - .collect(Collectors.toList()); - flush(); - } - - private void freeSlotAndFailPromise(Slot slot, Promise promise, Throwable x) - { - synchronized (this) + try (Locker.Lock l = lock.lock()) { slots.remove(slot); } + HTTP2Session.this.onStreamDestroyed(streamId); flush(); - promise.failed(x); } /** @@ -1821,7 +2176,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio while (true) { List entries; - synchronized (this) + try (Locker.Lock l = lock.lock()) { if (flushing == null) flushing = thread; @@ -1845,6 +2200,21 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio flusher.iterate(); } + @Override + public String toString() + { + try (Locker.Lock l = lock.lock()) + { + return String.format("state=[streams=%d,%s,goAwayRecv=%s,goAwaySent=%s,failure=%s]", + streamCount.get(), + closed, + goAwayRecv, + goAwaySent, + failure + ); + } + } + private class Slot { private volatile List entries; 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 d0b108230ff..6531d05afea 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 @@ -326,21 +326,11 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa length = fields.getLongField(HttpHeader.CONTENT_LENGTH.asString()); dataLength = length >= 0 ? length : Long.MIN_VALUE; } - callback.succeeded(); } private void onData(DataFrame frame, Callback callback) { - if (getRecvWindow() < 0) - { - // It's a bad client, it does not deserve to be - // treated gently by just resetting the stream. - session.close(ErrorCode.FLOW_CONTROL_ERROR.code, "stream_window_exceeded", Callback.NOOP); - callback.failed(new IOException("stream_window_exceeded")); - return; - } - // SPEC: remotely closed streams must be replied with a reset. if (isRemotelyClosed()) { @@ -381,8 +371,8 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa failure = new EofException("reset"); } close(); - session.removeStream(this); - notifyReset(this, frame, callback); + if (session.removeStream(this)) + notifyReset(this, frame, callback); } private void onPush(PushPromiseFrame frame, Callback callback) @@ -405,8 +395,8 @@ public class HTTP2Stream extends IdleTimeout implements IStream, Callback, Dumpa failure = frame.getFailure(); } close(); - session.removeStream(this); - notifyFailure(this, frame, callback); + if (session.removeStream(this)) + notifyFailure(this, frame, callback); } @Override diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/ISession.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/ISession.java index 412e6808722..430d0269b4b 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/ISession.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/ISession.java @@ -44,8 +44,9 @@ public interface ISession extends Session *

Removes the given {@code stream}.

* * @param stream the stream to remove + * @return whether the stream was removed */ - void removeStream(IStream stream); + boolean removeStream(IStream stream); /** *

Sends the given list of frames to create a new {@link Stream}.

diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Session.java index 3406d9f0453..8033dbfefbb 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/api/Session.java @@ -97,8 +97,6 @@ public interface Session /** *

Closes the session by sending a GOAWAY frame with the given error code * and payload.

- *

The GOAWAY frame is sent only once; subsequent or concurrent attempts to - * close the session will have no effect.

* * @param error the error code * @param payload an optional payload (may be null) @@ -197,6 +195,16 @@ public interface Session * * @param session the session * @param frame the GOAWAY frame received + */ + default void onGoAway(Session session, GoAwayFrame frame) + { + } + + /** + *

Callback method invoked when a GOAWAY frame caused the session to be closed.

+ * + * @param session the session + * @param frame the GOAWAY frame that caused the session to be closed * @param callback the callback to notify of the GOAWAY processing */ default void onClose(Session session, GoAwayFrame frame, Callback callback) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/GoAwayFrame.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/GoAwayFrame.java index bbd1bcdb703..743eb07b056 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/GoAwayFrame.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/frames/GoAwayFrame.java @@ -20,30 +20,33 @@ package org.eclipse.jetty.http2.frames; import java.nio.charset.StandardCharsets; -import org.eclipse.jetty.http2.CloseState; import org.eclipse.jetty.http2.ErrorCode; public class GoAwayFrame extends Frame { - private final CloseState closeState; + public static final GoAwayFrame GRACEFUL = new GoAwayFrame(Integer.MAX_VALUE, ErrorCode.NO_ERROR.code, new byte[]{'g', 'r', 'a', 'c', 'e', 'f', 'u', 'l'}); + private final int lastStreamId; private final int error; private final byte[] payload; public GoAwayFrame(int lastStreamId, int error, byte[] payload) - { - this(CloseState.REMOTELY_CLOSED, lastStreamId, error, payload); - } - - public GoAwayFrame(CloseState closeState, int lastStreamId, int error, byte[] payload) { super(FrameType.GO_AWAY); - this.closeState = closeState; this.lastStreamId = lastStreamId; this.error = error; this.payload = payload; } + /** + * @return whether this GOAWAY frame is graceful, i.e. its {@code lastStreamId == Integer.MAX_VALUE} + */ + public boolean isGraceful() + { + // SPEC: section 6.8. + return lastStreamId == Integer.MAX_VALUE; + } + public int getLastStreamId() { return lastStreamId; @@ -76,11 +79,10 @@ public class GoAwayFrame extends Frame @Override public String toString() { - return String.format("%s,%d/%s/%s/%s", + return String.format("%s{%d/%s/%s}", super.toString(), lastStreamId, ErrorCode.toString(error, null), - tryConvertPayload(), - closeState); + tryConvertPayload()); } } diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java index b31b6632d76..1f2fd05daa6 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java @@ -35,6 +35,7 @@ public class AbstractTest { protected Server server; protected ServerConnector connector; + protected HTTP2Client http2Client; protected HttpClient client; protected void start(ServerSessionListener listener) throws Exception @@ -63,12 +64,13 @@ public class AbstractTest server.addConnector(connector); } - protected void prepareClient() throws Exception + protected void prepareClient() { - client = new HttpClient(new HttpClientTransportOverHTTP2(new HTTP2Client()), null); + http2Client = new HTTP2Client(); + client = new HttpClient(new HttpClientTransportOverHTTP2(http2Client), null); QueuedThreadPool clientExecutor = new QueuedThreadPool(); clientExecutor.setName("client"); - client.setExecutor(clientExecutor); + this.client.setExecutor(clientExecutor); } @AfterEach diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java index 664e5a0fed9..bb89c05a8b3 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java @@ -220,7 +220,9 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest MetaData.Request request = (MetaData.Request)frame.getMetaData(); if (HttpMethod.HEAD.is(request.getMethod())) { - stream.getSession().close(ErrorCode.REFUSED_STREAM_ERROR.code, null, Callback.NOOP); + int error = ErrorCode.REFUSED_STREAM_ERROR.code; + stream.reset(new ResetFrame(stream.getId(), error), Callback.NOOP); + stream.getSession().close(error, null, Callback.NOOP); } else { diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/RawHTTP2ServerConnectionFactory.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/RawHTTP2ServerConnectionFactory.java index 7f8a4d9a659..a1c44493584 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/RawHTTP2ServerConnectionFactory.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/RawHTTP2ServerConnectionFactory.java @@ -104,6 +104,12 @@ public class RawHTTP2ServerConnectionFactory extends AbstractHTTP2ServerConnecti delegate.onReset(session, frame); } + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + delegate.onGoAway(session, frame); + } + @Override public void onClose(Session session, GoAwayFrame frame) { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index d8ab170f61a..576853ee85f 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -503,6 +503,6 @@ public abstract class IteratingCallback implements Callback @Override public String toString() { - return String.format("%s[%s]", super.toString(), _state); + return String.format("%s@%x[%s]", getClass().getSimpleName(), hashCode(), _state); } } From 1de622f72fdc315f610b8cebfa5dae716fe492b4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 10 Nov 2020 15:25:44 +0100 Subject: [PATCH 06/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Updates after initial review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 94 +++++++++---------- 1 file changed, 46 insertions(+), 48 deletions(-) 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 01f9eaefe85..2fe019468ed 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 @@ -943,7 +943,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public void onShutdown() { - streamsState.onShutdown(this); + streamsState.onShutdown(); } /** @@ -970,7 +970,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio @Override public boolean onIdleTimeout() { - return streamsState.onIdleTimeout(this); + return streamsState.onIdleTimeout(); } private void notIdle() @@ -984,28 +984,28 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "upgrade"); } - protected void onStreamCreated(int streamId) + private void onStreamCreated(int streamId) { if (LOG.isDebugEnabled()) LOG.debug("Created stream #{} for {}", streamId, this); streamsState.onStreamCreated(); } - protected void onStreamOpened(IStream stream) + protected final void onStreamOpened(IStream stream) { if (LOG.isDebugEnabled()) LOG.debug("Opened stream {} for {}", stream, this); streamsOpened.incrementAndGet(); } - protected void onStreamClosed(IStream stream) + private void onStreamClosed(IStream stream) { if (LOG.isDebugEnabled()) LOG.debug("Closed stream {} for {}", stream, this); streamsClosed.incrementAndGet(); } - protected void onStreamDestroyed(int streamId) + private void onStreamDestroyed(int streamId) { if (LOG.isDebugEnabled()) LOG.debug("Destroyed stream #{} for {}", streamId, this); @@ -1522,7 +1522,10 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } else { - if (goAwaySent.isGraceful()) + // SPEC: see section 6.8. + if (goAwaySent.isGraceful() || + frame.getLastStreamId() < goAwaySent.getLastStreamId() || + frame.getError() != ErrorCode.NO_ERROR.code) { goAwaySent = frame; sendGoAway = true; @@ -1613,7 +1616,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } frame = goAwaySent; closed = CloseState.CLOSED; - failure = toFailure(ErrorCode.NO_ERROR.code, reason); + if (failure != null) + failure = toFailure(ErrorCode.NO_ERROR.code, reason); break; } default: @@ -1726,10 +1730,10 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio tryRunClosingAction(); } - private void onShutdown(HTTP2Session session) + private void onShutdown() { String reason = "input_shutdown"; - boolean resetStreams = false; + boolean failStreams = false; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1738,8 +1742,9 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case LOCALLY_CLOSED: { if (LOG.isDebugEnabled()) - LOG.debug("Unexpected ISHUT for {}", session); + LOG.debug("Unexpected ISHUT for {}", HTTP2Session.this); closed = CloseState.CLOSING; + // Don't record the GOAWAY received because there is no lastStreamId. GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); closingAction = () -> terminate(frame); failure = new ClosedChannelException(); @@ -1748,29 +1753,30 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case REMOTELY_CLOSED: { closed = CloseState.CLOSING; + // Don't record the GOAWAY received because there is no lastStreamId. GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); closingAction = () -> terminate(frame); failure = new ClosedChannelException(); - resetStreams = true; + failStreams = true; break; } case CLOSING: { if (failure == null) failure = new ClosedChannelException(); - resetStreams = true; + failStreams = true; break; } default: { if (LOG.isDebugEnabled()) - LOG.debug("Already closed, ignoring ISHUT for {}", session); + LOG.debug("Already closed, ignoring ISHUT for {}", HTTP2Session.this); return; } } } - if (resetStreams) + if (failStreams) { // Since nothing else will arrive from the other peer, reset // the streams for which the other peer did not send all frames. @@ -1784,7 +1790,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } - private boolean onIdleTimeout(HTTP2Session session) + private boolean onIdleTimeout() { String reason = "idle_timeout"; boolean notify = false; @@ -1801,7 +1807,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio notify = true; break; } - // Timed out while waiting for closing events. + // Timed out while waiting for closing events, abort all the streams. case LOCALLY_CLOSED: { boolean shouldSend = goAwaySent.isGraceful(); @@ -1822,7 +1828,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio default: { if (LOG.isDebugEnabled()) - LOG.debug("Already closed, ignored idle timeout for {}", session); + LOG.debug("Already closed, ignored idle timeout for {}", HTTP2Session.this); return false; } } @@ -1830,9 +1836,9 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (notify) { - boolean close = notifyIdleTimeout(session); + boolean close = notifyIdleTimeout(HTTP2Session.this); if (LOG.isDebugEnabled()) - LOG.debug("Idle timeout {} for {}", close ? "confirmed" : "ignored", session); + LOG.debug("Idle timeout {} for {}", close ? "confirmed" : "ignored", HTTP2Session.this); if (close) halt(reason); return false; @@ -1930,25 +1936,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private void onStreamDestroyed() { long count = streamCount.decrementAndGet(); + // I've seen zero here, but it may increase again. + // That's why tryRunClosingAction() must check + // the count with the lock held. if (count == 0) - runClosingAction(); + tryRunClosingAction(); } private void tryRunClosingAction() - { - long count = streamCount.get(); - if (count == 0) - { - runClosingAction(); - } - else - { - if (LOG.isDebugEnabled()) - LOG.debug("Deferred closing action, {} pending streams on {}", count, HTTP2Session.this); - } - } - - private void runClosingAction() { // Threads from onStreamClosed() and other events // such as onGoAway() may be in a race to finish, @@ -1956,6 +1951,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio Runnable action = null; try (Locker.Lock l = lock.lock()) { + long count = streamCount.get(); + if (count > 0) + { + if (LOG.isDebugEnabled()) + LOG.debug("Deferred closing action, {} pending streams on {}", count, HTTP2Session.this); + return; + } + switch (closed) { case LOCALLY_CLOSED: @@ -2115,14 +2118,16 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private int reserveSlot(Slot slot, int streamId, Consumer fail) { - int newStreamId = streamId; Throwable failure = null; try (Locker.Lock l = lock.lock()) { if (closed == CloseState.NOT_CLOSED) { - if (newStreamId <= 0) - newStreamId = localStreamIds.getAndAdd(2); + if (streamId <= 0) + { + streamId = localStreamIds.getAndAdd(2); + HTTP2Session.this.onStreamCreated(streamId); + } slots.offer(slot); } else @@ -2133,16 +2138,9 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } if (failure == null) - { - if (streamId <= 0) - HTTP2Session.this.onStreamCreated(newStreamId); - return newStreamId; - } - else - { - fail.accept(failure); - return 0; - } + return streamId; + fail.accept(failure); + return 0; } private void freeSlot(Slot slot, int streamId) From a02012fb3e3dcf8e10454843aa91ce21a6fb7626 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 10 Nov 2020 16:17:32 +0100 Subject: [PATCH 07/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Fixed logic in onShutdown(): in case of abort the closing action must be run as callback, so it executes at the end of all the other callbacks. Signed-off-by: Simone Bordet --- .../main/java/org/eclipse/jetty/http2/HTTP2Session.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) 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 2fe019468ed..9ad5b36add5 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 @@ -1744,16 +1744,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (LOG.isDebugEnabled()) LOG.debug("Unexpected ISHUT for {}", HTTP2Session.this); closed = CloseState.CLOSING; - // Don't record the GOAWAY received because there is no lastStreamId. - GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); - closingAction = () -> terminate(frame); failure = new ClosedChannelException(); break; } case REMOTELY_CLOSED: { closed = CloseState.CLOSING; - // Don't record the GOAWAY received because there is no lastStreamId. GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); closingAction = () -> terminate(frame); failure = new ClosedChannelException(); @@ -1786,7 +1782,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } else { - abort(reason, failure, Callback.from(this::tryRunClosingAction)); + GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); + abort(reason, failure, Callback.from(() -> terminate(frame))); } } @@ -1905,7 +1902,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case REMOTELY_CLOSED: { closed = CloseState.CLOSING; - closingAction = () -> terminate(newGoAwayFrame(ErrorCode.NO_ERROR.code, reason)); + closingAction = () -> terminate(new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8))); failure = x; break; } From 298ddfd722a1aff207afe42f127adfa82a6e5634 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 10 Nov 2020 17:38:05 +0100 Subject: [PATCH 08/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Fixed javadocs and TODOs. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 84 ++++--------------- 1 file changed, 16 insertions(+), 68 deletions(-) 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 9ad5b36add5..e9c22320cf8 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 @@ -429,22 +429,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } /** - * This method is called when receiving a GO_AWAY from the other peer. - * We check the close state to act appropriately: - *
    - *
  • NOT_CLOSED: we move to REMOTELY_CLOSED and queue a disconnect, so - * that the content of the queue is written, and then the connection - * closed. We notify the application after being terminated. - * See {@code HTTP2Session.ControlEntry#succeeded()}
    • - *
  • In all other cases, we do nothing since other methods are already - * performing their actions.
    • - *
+ *

This method is called when receiving a GO_AWAY from the other peer.

* * @param frame the GO_AWAY frame that has been received. * @see #close(int, String, Callback) * @see #onShutdown() * @see #onIdleTimeout() - * TODO: Review javadocs */ @Override public void onGoAway(GoAwayFrame frame) @@ -636,20 +626,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } /** - * Invoked internally and by applications to send a GO_AWAY frame to the - * other peer. We check the close state to act appropriately: - *
    - *
  • NOT_CLOSED: we move to LOCALLY_CLOSED and queue a GO_AWAY. When the - * GO_AWAY has been written, it will only cause the output to be shut - * down (not the connection closed), so that the application can still - * read frames arriving from the other peer. - * Ideally the other peer will notice the GO_AWAY and close the connection. - * When that happen, we close the connection from {@link #onShutdown()}. - * Otherwise, the idle timeout mechanism will close the connection, see - * {@link #onIdleTimeout()}.
    • - *
  • In all other cases, we do nothing since other methods are already - * performing their actions.
    • - *
+ *

Invoked internally and by applications to send a GO_AWAY frame to the other peer.

* * @param error the error code * @param reason the reason @@ -657,7 +634,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio * @see #onGoAway(GoAwayFrame) * @see #onShutdown() * @see #onIdleTimeout() - * // TODO: review javadocs */ @Override public boolean close(int error, String reason, Callback callback) @@ -919,26 +895,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } /** - * A typical close by a remote peer involves a GO_AWAY frame followed by TCP FIN. - * This method is invoked when the TCP FIN is received, or when an exception is - * thrown while reading, and we check the close state to act appropriately: - *
    - *
  • NOT_CLOSED: means that the remote peer did not send a GO_AWAY (abrupt close) - * or there was an exception while reading, and therefore we terminate.
    • - *
  • LOCALLY_CLOSED: we have sent the GO_AWAY to the remote peer, which received - * it and closed the connection; we queue a disconnect to close the connection - * on the local side. - * The GO_AWAY just shutdown the output, so we need this step to make sure the - * connection is closed. See {@link #close(int, String, Callback)}.
    • - *
  • REMOTELY_CLOSED: we received the GO_AWAY, and the TCP FIN afterwards, so we - * do nothing since the handling of the GO_AWAY will take care of closing the - * connection. See {@link #onGoAway(GoAwayFrame)}.
    • - *
+ *

This method is called when the TCP FIN is received from the remote peer.

* * @see #onGoAway(GoAwayFrame) * @see #close(int, String, Callback) * @see #onIdleTimeout() - * TODO: review javadocs */ @Override public void onShutdown() @@ -947,25 +908,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } /** - * This method is invoked when the idle timeout triggers. We check the close state - * to act appropriately: - *
    - *
  • NOT_CLOSED: it's a real idle timeout, we just initiate a close, see - * {@link #close(int, String, Callback)}.
    • - *
  • LOCALLY_CLOSED: we have sent a GO_AWAY and only shutdown the output, but the - * other peer did not close the connection so we never received the TCP FIN, and - * therefore we terminate.
    • - *
  • REMOTELY_CLOSED: the other peer sent us a GO_AWAY, we should have queued a - * disconnect, but for some reason it was not processed (for example, queue was - * stuck because of TCP congestion), therefore we terminate. - * See {@link #onGoAway(GoAwayFrame)}.
    • - *
+ *

This method is invoked when the idle timeout expires.

* * @return true if the session should be closed, false otherwise * @see #onGoAway(GoAwayFrame) * @see #close(int, String, Callback) * @see #onShutdown() - * TODO: review javadocs */ @Override public boolean onIdleTimeout() @@ -1457,13 +1405,15 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } /** - * SPEC: It is required that stream ids are monotonically increasing. - * Here we use a queue to atomically create the stream id and - * claim the slot in the queue. Concurrent threads will only - * flush up to the slot with a non-null entry to make sure - * frames are sent strictly in their stream id order. - * See https://tools.ietf.org/html/rfc7540#section-5.1.1. - * TODO: javadocs + *

The HTTP/2 specification requires that stream ids are monotonically increasing, + * see https://tools.ietf.org/html/rfc7540#section-5.1.1.

+ *

This implementation uses a queue to atomically reserve a stream id and claim + * a slot in the queue; the slot is then assigned the entries to write.

+ *

Concurrent threads push slots in the queue but only one thread flushes + * the slots, up to the slot that has a non-null entries to write, therefore + * guaranteeing that frames are sent strictly in their stream id order.

+ *

This class also coordinates the creation of streams with the close of + * the session, see https://tools.ietf.org/html/rfc7540#section-6.8.

*/ private class StreamsState { @@ -1475,7 +1425,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private Runnable closingAction; private GoAwayFrame goAwayRecv; private GoAwayFrame goAwaySent; - private Throwable failure; + private volatile Throwable failure; private Thread flushing; private CloseState getCloseState() @@ -1505,9 +1455,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio // when the last stream is destroyed. closingAction = action = () -> { - // TODO: verify this GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); - goAway(goAwayFrame, Callback.from(Callback.NOOP::succeeded, HTTP2Session.this::terminate)); + goAway(goAwayFrame, Callback.NOOP); }; } break; @@ -1549,9 +1498,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio // when the last stream is destroyed. closingAction = action = () -> { - // TODO: verify this GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); - goAway(goAwayFrame, Callback.from(Callback.NOOP::succeeded, HTTP2Session.this::terminate)); + goAway(goAwayFrame, Callback.NOOP); }; } else From 96e4b38624a4288dc6d176aeead5cde470b3c63a Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 12 Nov 2020 17:13:17 +0100 Subject: [PATCH 09/11] Fixes #5633 - Allow to configure HttpClient request authority. Fixed initial session recv window update: it was wrong if the initial value was less than the default value (65535). Signed-off-by: Simone Bordet --- .../http2/client/HTTP2ClientConnectionFactory.java | 6 +----- .../java/org/eclipse/jetty/http2/HTTP2Session.java | 2 +- .../jetty/http2/server/HTTP2ServerSession.java | 12 +++--------- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java index 388599be9b1..8c3e2e99fdd 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java @@ -111,15 +111,11 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory ISession session = getSession(); int windowDelta = client.getInitialSessionRecvWindow() - FlowControlStrategy.DEFAULT_WINDOW_SIZE; + session.updateRecvWindow(windowDelta); if (windowDelta > 0) - { - session.updateRecvWindow(windowDelta); session.frames(null, Arrays.asList(prefaceFrame, settingsFrame, new WindowUpdateFrame(0, windowDelta)), this); - } else - { session.frames(null, Arrays.asList(prefaceFrame, settingsFrame), this); - } } @Override 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 e9c22320cf8..ecee9d19988 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 @@ -376,7 +376,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case SettingsFrame.INITIAL_WINDOW_SIZE: { if (LOG.isDebugEnabled()) - LOG.debug("Updating initial window size to {} for {}", value, this); + LOG.debug("Updating initial stream window size to {} for {}", value, this); flowControl.updateInitialStreamWindow(this, value, false); break; } diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java index 6e87bf17675..26808f8fd6d 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java @@ -65,18 +65,12 @@ public class HTTP2ServerSession extends HTTP2Session implements ServerParser.Lis settings = Collections.emptyMap(); SettingsFrame settingsFrame = new SettingsFrame(settings, false); - WindowUpdateFrame windowFrame = null; int sessionWindow = getInitialSessionRecvWindow() - FlowControlStrategy.DEFAULT_WINDOW_SIZE; + updateRecvWindow(sessionWindow); if (sessionWindow > 0) - { - updateRecvWindow(sessionWindow); - windowFrame = new WindowUpdateFrame(0, sessionWindow); - } - - if (windowFrame == null) - frames(null, Collections.singletonList(settingsFrame), Callback.NOOP); + frames(null, Arrays.asList(settingsFrame, new WindowUpdateFrame(0, sessionWindow)), Callback.NOOP); else - frames(null, Arrays.asList(settingsFrame, windowFrame), Callback.NOOP); + frames(null, Collections.singletonList(settingsFrame), Callback.NOOP); } @Override From dd9bdc7ac376c19ffff6ff927b65c3781da7323c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 12 Nov 2020 17:13:36 +0100 Subject: [PATCH 10/11] Fixes #5633 - Allow to configure HttpClient request authority. Updated after review. Added more tests. Signed-off-by: Simone Bordet --- .../jetty/http2/client/GoAwayTest.java | 152 ++++++++++++++++-- .../org/eclipse/jetty/http2/HTTP2Session.java | 37 +++-- 2 files changed, 166 insertions(+), 23 deletions(-) diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java index a2c2345d6e2..b7018a11c35 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/GoAwayTest.java @@ -18,9 +18,11 @@ package org.eclipse.jetty.http2.client; +import java.nio.ByteBuffer; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.http.HttpFields; @@ -30,7 +32,11 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.CloseState; import org.eclipse.jetty.http2.ErrorCode; +import org.eclipse.jetty.http2.FlowControlStrategy; import org.eclipse.jetty.http2.HTTP2Session; +import org.eclipse.jetty.http2.ISession; +import org.eclipse.jetty.http2.IStream; +import org.eclipse.jetty.http2.SimpleFlowControlStrategy; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; import org.eclipse.jetty.http2.api.server.ServerSessionListener; @@ -173,7 +179,7 @@ public class GoAwayTest extends AbstractTest }); Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); -// Assertions.assertTrue(streamFailureLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(streamFailureLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); @@ -354,9 +360,125 @@ public class GoAwayTest extends AbstractTest // The server should have sent the GOAWAY after the last stream completed. Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + + Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); + Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); + } + + @Test + public void testServerGoAwayWithStalledStreamServerConsumesDataOfInFlightStream() throws Exception + { + int flowControlWindow = 32 * 1024; + + AtomicReference serverSessionRef = new AtomicReference<>(); + CountDownLatch serverGoAwayLatch = new CountDownLatch(1); + CountDownLatch serverCloseLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onAccept(Session session) + { + serverSessionRef.set(session); + } + + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + AtomicInteger dataFrames = new AtomicInteger(); + return new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame frame, Callback callback) + { + // Do not consume the data for this stream (i.e. don't succeed the callback). + // Only send the response when receiving the first DATA frame. + if (dataFrames.incrementAndGet() == 1) + { + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + stream.headers(new HeadersFrame(stream.getId(), response, null, true), Callback.NOOP); + } + } + }; + } + + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + serverGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + serverCloseLatch.countDown(); + } + }, h2 -> + { + // Use the simple, predictable, strategy for window updates. + h2.setFlowControlStrategyFactory(SimpleFlowControlStrategy::new); + h2.setInitialSessionRecvWindow(flowControlWindow); + h2.setInitialStreamRecvWindow(flowControlWindow); + }); + + CountDownLatch clientGoAwayLatch = new CountDownLatch(1); + CountDownLatch clientCloseLatch = new CountDownLatch(1); + Session clientSession = newClient(new Session.Listener.Adapter() + { + @Override + public void onGoAway(Session session, GoAwayFrame frame) + { + clientGoAwayLatch.countDown(); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + clientCloseLatch.countDown(); + } + }); + // This is necessary because the server session window is smaller than the + // default and the server cannot send a WINDOW_UPDATE with a negative value. + ((ISession)clientSession).updateSendWindow(flowControlWindow - FlowControlStrategy.DEFAULT_WINDOW_SIZE); + + MetaData.Request request1 = newRequest("GET", new HttpFields()); + HeadersFrame headersFrame1 = new HeadersFrame(request1, null, false); + DataFrame dataFrame1 = new DataFrame(ByteBuffer.allocate(flowControlWindow / 2), false); + ((ISession)clientSession).newStream(new IStream.FrameList(headersFrame1, dataFrame1, null), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream clientStream1, HeadersFrame frame) + { + // Send the server GOAWAY frame. + serverSessionRef.get().close(ErrorCode.NO_ERROR.code, null, Callback.NOOP); + + // Send a second, in-flight, stream with data, which + // will exhaust the client session flow control window. + // The server should consume the data even if it will drop + // this stream, so that the first stream can send more data. + MetaData.Request request2 = newRequest("POST", new HttpFields()); + HeadersFrame headersFrame2 = new HeadersFrame(request2, null, false); + DataFrame dataFrame2 = new DataFrame(ByteBuffer.allocate(flowControlWindow / 2), true); + ((ISession)clientStream1.getSession()).newStream(new IStream.FrameList(headersFrame2, dataFrame2, null), new Promise.Adapter() + { + @Override + public void succeeded(Stream clientStream2) + { + // After the in-flight stream is sent, try to complete the first stream. + // The client should receive the window update from + // the server and be able to complete this stream. + clientStream1.data(new DataFrame(clientStream1.getId(), ByteBuffer.allocate(flowControlWindow / 2), true), Callback.NOOP); + } + }, new Adapter()); + } + }); + + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverGoAwayLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); Assertions.assertFalse(((HTTP2Session)clientSession).getEndPoint().isOpen()); @@ -643,6 +765,7 @@ public class GoAwayTest extends AbstractTest }); Session clientSession = newClient(new Session.Listener.Adapter()); + // TODO: get rid of sleep! // Wait for the SETTINGS frames to be exchanged. Thread.sleep(500); @@ -691,6 +814,8 @@ public class GoAwayTest extends AbstractTest Assertions.assertFalse(((HTTP2Session)serverSessionRef.get()).getEndPoint().isOpen()); } + // TODO: add a shutdown test with pending stream. + @Test public void testServerIdleTimeout() throws Exception { @@ -729,7 +854,8 @@ public class GoAwayTest extends AbstractTest @Override public void onGoAway(Session session, GoAwayFrame frame) { - clientGoAwayLatch.countDown(); + if (!frame.isGraceful()) + clientGoAwayLatch.countDown(); } @Override @@ -802,13 +928,22 @@ public class GoAwayTest extends AbstractTest clientCloseLatch.countDown(); } }); + CountDownLatch clientResetLatch = new CountDownLatch(1); MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); // Send request headers but not data. - clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter()); + clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onReset(Stream stream, ResetFrame frame) + { + clientResetLatch.countDown(); + } + }); Assertions.assertTrue(clientGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); // Server idle timeout sends a non-graceful GOAWAY. - Assertions.assertTrue(clientGoAwayLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assertions.assertTrue(clientResetLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); @@ -871,14 +1006,13 @@ public class GoAwayTest extends AbstractTest } }); MetaData.Request request = newRequest(HttpMethod.GET.asString(), new HttpFields()); - CountDownLatch streamFailureLatch = new CountDownLatch(1); + CountDownLatch streamResetLatch = new CountDownLatch(1); clientSession.newStream(new HeadersFrame(request, null, false), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override - public void onFailure(Stream stream, int error, String reason, Throwable failure, Callback callback) + public void onReset(Stream stream, ResetFrame frame) { - streamFailureLatch.countDown(); - callback.succeeded(); + streamResetLatch.countDown(); } }); @@ -886,8 +1020,8 @@ public class GoAwayTest extends AbstractTest ((HTTP2Session)clientSession).goAway(GoAwayFrame.GRACEFUL, Callback.NOOP); Assertions.assertTrue(serverGracefulGoAwayLatch.await(5, TimeUnit.SECONDS)); + Assertions.assertTrue(streamResetLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(clientGoAwayLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); - Assertions.assertTrue(streamFailureLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS)); Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS)); 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 ecee9d19988..43f3ce97925 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 @@ -757,6 +757,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio protected IStream createRemoteStream(int streamId) { + // This stream has been seen the server. + // Even if the stream cannot be created because this peer is closing, + // updating the lastRemoteStreamId ensures that in-flight HEADERS and + // DATA frames can be read (and discarded) without causing an error. + updateLastRemoteStreamId(streamId); + if (!streamsState.newRemoteStream(streamId)) { if (LOG.isDebugEnabled()) @@ -764,9 +770,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio return null; } - // This stream has been seen the server. - updateLastRemoteStreamId(streamId); - // SPEC: exceeding max concurrent streams is treated as stream error. while (true) { @@ -1423,8 +1426,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private long idleTime = System.nanoTime(); private CloseState closed = CloseState.NOT_CLOSED; private Runnable closingAction; - private GoAwayFrame goAwayRecv; - private GoAwayFrame goAwaySent; + private volatile GoAwayFrame goAwayRecv; + private volatile GoAwayFrame goAwaySent; private volatile Throwable failure; private Thread flushing; @@ -1739,7 +1742,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { String reason = "idle_timeout"; boolean notify = false; - Throwable cause = null; + boolean sendGoAway = false; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1752,22 +1755,24 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio notify = true; break; } - // Timed out while waiting for closing events, abort all the streams. + // Timed out while waiting for closing events, fail all the streams. case LOCALLY_CLOSED: { - boolean shouldSend = goAwaySent.isGraceful(); - goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + if (goAwaySent.isGraceful()) + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + sendGoAway = true; + } closed = CloseState.CLOSING; - closingAction = shouldSend ? () -> sendGoAwayAndTerminate(goAwaySent) : () -> terminate(goAwaySent); - failure = cause = new TimeoutException("Session idle timeout expired"); + failure = new TimeoutException("Session idle timeout expired"); break; } case REMOTELY_CLOSED: { goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); closed = CloseState.CLOSING; - closingAction = () -> sendGoAwayAndTerminate(goAwaySent); - failure = cause = new TimeoutException("Session idle timeout expired"); + failure = new TimeoutException("Session idle timeout expired"); + sendGoAway = true; break; } default: @@ -1789,7 +1794,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio return false; } - abort(reason, cause, Callback.from(this::tryRunClosingAction)); + failStreams(stream -> true, reason, true); + if (sendGoAway) + sendGoAway(goAwaySent, Callback.NOOP); + notifyFailure(HTTP2Session.this, failure, Callback.NOOP); + terminate(goAwaySent); return false; } From 4093af1824a4cad91c1777e8064cd88b92ba35c4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 18 Nov 2020 12:03:35 +0100 Subject: [PATCH 11/11] Issue 5310 - Review HTTP/2 GOAWAY handling. Updates after review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 187 ++++++++++-------- .../test/resources/jetty-logging.properties | 2 +- 2 files changed, 106 insertions(+), 83 deletions(-) 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 43f3ce97925..cd870eb4b40 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 @@ -649,6 +649,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } private GoAwayFrame newGoAwayFrame(int error, String reason) + { + return newGoAwayFrame(getLastRemoteStreamId(), error, reason); + } + + private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String reason) { byte[] payload = null; if (reason != null) @@ -657,7 +662,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio reason = reason.substring(0, Math.min(reason.length(), 32)); payload = reason.getBytes(StandardCharsets.UTF_8); } - return new GoAwayFrame(getLastRemoteStreamId(), error, payload); + return new GoAwayFrame(lastRemoteStreamId, error, payload); } @Override @@ -1422,13 +1427,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { private final Locker lock = new Locker(); private final Queue slots = new ArrayDeque<>(); + // Must be incremented with the lock held. private final AtomicLong streamCount = new AtomicLong(); private long idleTime = System.nanoTime(); private CloseState closed = CloseState.NOT_CLOSED; - private Runnable closingAction; - private volatile GoAwayFrame goAwayRecv; - private volatile GoAwayFrame goAwaySent; - private volatile Throwable failure; + private Runnable zeroStreamsAction; + private GoAwayFrame goAwayRecv; + private GoAwayFrame goAwaySent; + private Throwable failure; private Thread flushing; private CloseState getCloseState() @@ -1441,8 +1447,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio private boolean goAway(GoAwayFrame frame, Callback callback) { - Runnable action = null; boolean sendGoAway = false; + boolean tryRunZeroStreamsAction = false; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1456,11 +1462,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { // Try to send the non-graceful GOAWAY // when the last stream is destroyed. - closingAction = action = () -> + zeroStreamsAction = () -> { GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); goAway(goAwayFrame, Callback.NOOP); }; + tryRunZeroStreamsAction = streamCount.get() == 0; } break; } @@ -1499,11 +1506,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { // Try to send the non-graceful GOAWAY // when the last stream is destroyed. - closingAction = action = () -> + zeroStreamsAction = () -> { GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); goAway(goAwayFrame, Callback.NOOP); }; + tryRunZeroStreamsAction = streamCount.get() == 0; } else { @@ -1515,7 +1523,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio else { closed = CloseState.CLOSING; - closingAction = action = () -> terminate(frame); + zeroStreamsAction = () -> terminate(frame); + tryRunZeroStreamsAction = streamCount.get() == 0; } } break; @@ -1532,10 +1541,10 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (sendGoAway) { - if (action == null) - sendGoAway(frame, callback); + if (tryRunZeroStreamsAction) + sendGoAway(frame, Callback.from(callback, this::tryRunZeroStreamsAction)); else - sendGoAway(frame, Callback.from(callback, this::tryRunClosingAction)); + sendGoAway(frame, callback); return true; } else @@ -1549,8 +1558,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { if (LOG.isDebugEnabled()) LOG.debug("Halting ({}) for {}", reason, HTTP2Session.this); - GoAwayFrame frame; - boolean sendGoAway = false; + GoAwayFrame goAwayFrame = null; + GoAwayFrame goAwayFrameEvent; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1561,12 +1570,10 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case CLOSING: { if (goAwaySent == null || goAwaySent.isGraceful()) - { - goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); - sendGoAway = true; - } - frame = goAwaySent; + goAwaySent = goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); + goAwayFrameEvent = goAwayRecv != null ? goAwayRecv : goAwaySent; closed = CloseState.CLOSED; + zeroStreamsAction = null; if (failure != null) failure = toFailure(ErrorCode.NO_ERROR.code, reason); break; @@ -1578,16 +1585,16 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } failStreams(stream -> true, reason, true); - if (sendGoAway) - sendGoAwayAndTerminate(frame); + if (goAwayFrame != null) + sendGoAwayAndTerminate(goAwayFrame, goAwayFrameEvent); else - terminate(frame); + terminate(goAwayFrameEvent); } private void onGoAway(GoAwayFrame frame) { boolean failStreams = false; - Runnable action = null; + boolean tryRunZeroStreamsAction = false; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1595,9 +1602,9 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case NOT_CLOSED: { goAwayRecv = frame; - closed = CloseState.REMOTELY_CLOSED; if (frame.isGraceful()) { + closed = CloseState.REMOTELY_CLOSED; if (LOG.isDebugEnabled()) LOG.debug("Waiting non-graceful GOAWAY for {}", HTTP2Session.this); } @@ -1605,7 +1612,9 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); closed = CloseState.CLOSING; - closingAction = action = () -> sendGoAwayAndTerminate(frame); + GoAwayFrame goAwayFrame = goAwaySent; + zeroStreamsAction = () -> sendGoAwayAndTerminate(goAwayFrame, frame); + tryRunZeroStreamsAction = streamCount.get() == 0; failStreams = true; } break; @@ -1621,16 +1630,18 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } else { + closed = CloseState.CLOSING; if (goAwaySent.isGraceful()) { goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); - closed = CloseState.CLOSING; - closingAction = action = () -> sendGoAwayAndTerminate(frame); + GoAwayFrame goAwayFrame = goAwaySent; + zeroStreamsAction = () -> sendGoAwayAndTerminate(goAwayFrame, frame); + tryRunZeroStreamsAction = streamCount.get() == 0; } else { - closed = CloseState.CLOSING; - closingAction = action = () -> terminate(frame); + zeroStreamsAction = () -> terminate(frame); + tryRunZeroStreamsAction = streamCount.get() == 0; failStreams = true; } } @@ -1646,11 +1657,19 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } else { - boolean shouldSend = goAwaySent == null || goAwaySent.isGraceful(); goAwayRecv = frame; - goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); closed = CloseState.CLOSING; - closingAction = action = shouldSend ? () -> sendGoAwayAndTerminate(frame) : () -> terminate(frame); + if (goAwaySent == null || goAwaySent.isGraceful()) + { + goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, "close"); + GoAwayFrame goAwayFrame = goAwaySent; + zeroStreamsAction = () -> sendGoAwayAndTerminate(goAwayFrame, frame); + } + else + { + zeroStreamsAction = () -> terminate(frame); + } + tryRunZeroStreamsAction = streamCount.get() == 0; failStreams = true; } break; @@ -1677,13 +1696,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio failStreams(failIf, "closing", false); } - if (action != null) - tryRunClosingAction(); + if (tryRunZeroStreamsAction) + tryRunZeroStreamsAction(); } private void onShutdown() { String reason = "input_shutdown"; + Throwable cause = null; boolean failStreams = false; try (Locker.Lock l = lock.lock()) { @@ -1695,14 +1715,14 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (LOG.isDebugEnabled()) LOG.debug("Unexpected ISHUT for {}", HTTP2Session.this); closed = CloseState.CLOSING; - failure = new ClosedChannelException(); + failure = cause = new ClosedChannelException(); break; } case REMOTELY_CLOSED: { closed = CloseState.CLOSING; - GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); - closingAction = () -> terminate(frame); + GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + zeroStreamsAction = () -> terminate(goAwayFrame); failure = new ClosedChannelException(); failStreams = true; break; @@ -1729,12 +1749,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio // the streams for which the other peer did not send all frames. Predicate failIf = stream -> !stream.isRemotelyClosed(); failStreams(failIf, reason, false); - tryRunClosingAction(); + tryRunZeroStreamsAction(); } else { - GoAwayFrame frame = new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8)); - abort(reason, failure, Callback.from(() -> terminate(frame))); + GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + abort(reason, cause, Callback.from(() -> terminate(goAwayFrame))); } } @@ -1743,6 +1763,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio String reason = "idle_timeout"; boolean notify = false; boolean sendGoAway = false; + GoAwayFrame goAwayFrame = null; + Throwable cause = null; try (Locker.Lock l = lock.lock()) { switch (closed) @@ -1763,16 +1785,20 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); sendGoAway = true; } + goAwayFrame = goAwaySent; closed = CloseState.CLOSING; - failure = new TimeoutException("Session idle timeout expired"); + zeroStreamsAction = null; + failure = cause = new TimeoutException("Session idle timeout expired"); break; } case REMOTELY_CLOSED: { goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); - closed = CloseState.CLOSING; - failure = new TimeoutException("Session idle timeout expired"); sendGoAway = true; + goAwayFrame = goAwaySent; + closed = CloseState.CLOSING; + zeroStreamsAction = null; + failure = cause = new TimeoutException("Session idle timeout expired"); break; } default: @@ -1786,50 +1812,39 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (notify) { - boolean close = notifyIdleTimeout(HTTP2Session.this); + boolean confirmed = notifyIdleTimeout(HTTP2Session.this); if (LOG.isDebugEnabled()) - LOG.debug("Idle timeout {} for {}", close ? "confirmed" : "ignored", HTTP2Session.this); - if (close) + LOG.debug("Idle timeout {} for {}", confirmed ? "confirmed" : "ignored", HTTP2Session.this); + if (confirmed) halt(reason); return false; } failStreams(stream -> true, reason, true); if (sendGoAway) - sendGoAway(goAwaySent, Callback.NOOP); - notifyFailure(HTTP2Session.this, failure, Callback.NOOP); - terminate(goAwaySent); + sendGoAway(goAwayFrame, Callback.NOOP); + notifyFailure(HTTP2Session.this, cause, Callback.NOOP); + terminate(goAwayFrame); return false; } private void onSessionFailure(int error, String reason, Callback callback) { + GoAwayFrame goAwayFrame; + Throwable cause; try (Locker.Lock l = lock.lock()) { switch (closed) { case NOT_CLOSED: - { - goAwayRecv = goAwaySent = newGoAwayFrame(error, reason); - closed = CloseState.CLOSING; - closingAction = () -> sendGoAwayAndTerminate(goAwaySent); - failure = toFailure(error, reason); - break; - } case LOCALLY_CLOSED: - { - goAwayRecv = newGoAwayFrame(error, reason); - closed = CloseState.CLOSING; - closingAction = goAwaySent.isGraceful() ? () -> sendGoAwayAndTerminate(goAwayRecv) : () -> terminate(goAwayRecv); - failure = toFailure(error, reason); - break; - } case REMOTELY_CLOSED: { - goAwaySent = newGoAwayFrame(error, reason); + // Send another GOAWAY with the error code. + goAwaySent = goAwayFrame = newGoAwayFrame(error, reason); closed = CloseState.CLOSING; - closingAction = () -> sendGoAwayAndTerminate(goAwaySent); - failure = toFailure(error, reason); + zeroStreamsAction = null; + failure = cause = toFailure(error, reason); break; } default: @@ -1843,8 +1858,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } if (LOG.isDebugEnabled()) - LOG.debug("Session failure {}", HTTP2Session.this, failure); - onFailure(error, reason, failure, Callback.from(callback, this::tryRunClosingAction)); + LOG.debug("Session failure {}", HTTP2Session.this, cause); + + failStreams(stream -> true, reason, true); + sendGoAway(goAwayFrame, Callback.NOOP); + notifyFailure(HTTP2Session.this, cause, Callback.NOOP); + terminate(goAwayFrame); } private void onWriteFailure(Throwable x) @@ -1859,7 +1878,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case REMOTELY_CLOSED: { closed = CloseState.CLOSING; - closingAction = () -> terminate(new GoAwayFrame(0, ErrorCode.NO_ERROR.code, reason.getBytes(StandardCharsets.UTF_8))); + zeroStreamsAction = () -> + { + GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + terminate(goAwayFrame); + }; failure = x; break; } @@ -1869,12 +1892,12 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio } } } - abort(reason, x, Callback.from(this::tryRunClosingAction)); + abort(reason, x, Callback.from(this::tryRunZeroStreamsAction)); } - private void sendGoAwayAndTerminate(GoAwayFrame frame) + private void sendGoAwayAndTerminate(GoAwayFrame frame, GoAwayFrame eventFrame) { - sendGoAway(frame, Callback.from(() -> terminate(frame))); + sendGoAway(frame, Callback.from(() -> terminate(eventFrame))); } private void sendGoAway(GoAwayFrame frame, Callback callback) @@ -1891,13 +1914,13 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { long count = streamCount.decrementAndGet(); // I've seen zero here, but it may increase again. - // That's why tryRunClosingAction() must check + // That's why tryRunZeroStreamsAction() must check // the count with the lock held. if (count == 0) - tryRunClosingAction(); + tryRunZeroStreamsAction(); } - private void tryRunClosingAction() + private void tryRunZeroStreamsAction() { // Threads from onStreamClosed() and other events // such as onGoAway() may be in a race to finish, @@ -1919,8 +1942,8 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { if (goAwaySent.isGraceful()) { - action = closingAction; - closingAction = null; + action = zeroStreamsAction; + zeroStreamsAction = null; } break; } @@ -1928,16 +1951,16 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio { if (goAwaySent != null && goAwaySent.isGraceful()) { - action = closingAction; - closingAction = null; + action = zeroStreamsAction; + zeroStreamsAction = null; } break; } case CLOSING: { closed = CloseState.CLOSED; - action = closingAction; - closingAction = null; + action = zeroStreamsAction; + zeroStreamsAction = null; break; } default: @@ -1949,7 +1972,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (action != null) { if (LOG.isDebugEnabled()) - LOG.debug("Executing closing action on {}", HTTP2Session.this); + LOG.debug("Executing zero streams action on {}", HTTP2Session.this); action.run(); } } diff --git a/jetty-http2/http2-http-client-transport/src/test/resources/jetty-logging.properties b/jetty-http2/http2-http-client-transport/src/test/resources/jetty-logging.properties index 34929219c9d..f27d75c6d40 100644 --- a/jetty-http2/http2-http-client-transport/src/test/resources/jetty-logging.properties +++ b/jetty-http2/http2-http-client-transport/src/test/resources/jetty-logging.properties @@ -1,6 +1,6 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.client.LEVEL=DEBUG -org.eclipse.jetty.http2.hpack.LEVEL=INFO #org.eclipse.jetty.http2.LEVEL=DEBUG +org.eclipse.jetty.http2.hpack.LEVEL=INFO #org.eclipse.jetty.io.ssl.LEVEL=DEBUG