From 107a4fff20276753a0275d863793c730964996e4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 11 Aug 2014 18:43:50 +0200 Subject: [PATCH] Fixed handling of INITIAL_WINDOW_SIZE setting. It must update only stream windows, and not the session window. --- .../client/HTTP2ClientConnectionFactory.java | 3 +- .../jetty/http2/client/FlowControlTest.java | 81 +++++++++++-------- .../org/eclipse/jetty/http2/FlowControl.java | 2 + .../eclipse/jetty/http2/HTTP2FlowControl.java | 5 +- .../org/eclipse/jetty/http2/HTTP2Session.java | 2 +- .../AbstractHTTP2ServerConnectionFactory.java | 3 +- 6 files changed, 56 insertions(+), 40 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 e03a8438e44..fd2b41b108a 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 @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Map; import java.util.concurrent.Executor; +import org.eclipse.jetty.http2.FlowControl; import org.eclipse.jetty.http2.HTTP2Connection; import org.eclipse.jetty.http2.HTTP2FlowControl; import org.eclipse.jetty.http2.ISession; @@ -59,7 +60,7 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory Promise promise = (Promise)context.get(SESSION_PROMISE_CONTEXT_KEY); Generator generator = new Generator(byteBufferPool, 4096); - HTTP2ClientSession session = new HTTP2ClientSession(scheduler, endPoint, generator, listener, new HTTP2FlowControl(65535)); + HTTP2ClientSession session = new HTTP2ClientSession(scheduler, endPoint, generator, listener, new HTTP2FlowControl(FlowControl.DEFAULT_WINDOW_SIZE)); Parser parser = new Parser(byteBufferPool, session, 4096, 8192); return new HTTP2ClientConnection(client, byteBufferPool, executor, endPoint, parser, session, 8192, promise, listener); } diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlTest.java index 4879e128eea..f92b36021b0 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/FlowControlTest.java @@ -32,6 +32,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.FlowControl; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; import org.eclipse.jetty.http2.api.server.ServerSessionListener; @@ -334,53 +335,54 @@ public class FlowControlTest extends AbstractTest public void testSessionStalledStallsNewStreams() throws Exception { final int windowSize = 1024; - final CountDownLatch settingsLatch = new CountDownLatch(1); startServer(new ServerSessionListener.Adapter() { - @Override - public void onSettings(Session session, SettingsFrame frame) - { - settingsLatch.countDown(); - } - @Override public Stream.Listener onNewStream(Stream stream, HeadersFrame requestFrame) { - // For every stream, send down half the window size of data. - MetaData.Response metaData = new MetaData.Response(HttpVersion.HTTP_2, 200, new HttpFields()); - HeadersFrame responseFrame = new HeadersFrame(stream.getId(), metaData, null, false); - stream.headers(responseFrame, Callback.Adapter.INSTANCE); - DataFrame dataFrame = new DataFrame(stream.getId(), ByteBuffer.allocate(windowSize / 2), true); - stream.data(dataFrame, Callback.Adapter.INSTANCE); - return null; + MetaData.Request request = (MetaData.Request)requestFrame.getMetaData(); + if ("POST".equalsIgnoreCase(request.getMethod())) + { + // Send data to consume the session window. + ByteBuffer data = ByteBuffer.allocate(FlowControl.DEFAULT_WINDOW_SIZE - windowSize); + DataFrame dataFrame = new DataFrame(stream.getId(), data, true); + stream.data(dataFrame, Callback.Adapter.INSTANCE); + return null; + } + else + { + // For every stream, send down half the window size of data. + MetaData.Response metaData = new MetaData.Response(HttpVersion.HTTP_2, 200, new HttpFields()); + HeadersFrame responseFrame = new HeadersFrame(stream.getId(), metaData, null, false); + stream.headers(responseFrame, Callback.Adapter.INSTANCE); + DataFrame dataFrame = new DataFrame(stream.getId(), ByteBuffer.allocate(windowSize / 2), true); + stream.data(dataFrame, Callback.Adapter.INSTANCE); + return null; + } } }); Session session = newClient(new Session.Listener.Adapter()); - Map settings = new HashMap<>(); - settings.put(SettingsFrame.INITIAL_WINDOW_SIZE, windowSize); - session.settings(new SettingsFrame(settings, false), Callback.Adapter.INSTANCE); - - Assert.assertTrue(settingsLatch.await(5, TimeUnit.SECONDS)); - - final AtomicReference callbackRef1 = new AtomicReference<>(); - final AtomicReference callbackRef2 = new AtomicReference<>(); - - // First request will consume half the session window. - MetaData.Request request1 = newRequest("GET", new HttpFields()); + // First request is just to consume the session window. + final CountDownLatch prepareLatch = new CountDownLatch(1); + MetaData.Request request1 = newRequest("POST", new HttpFields()); session.newStream(new HeadersFrame(0, request1, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) { - // Do not consume it to stall flow control. - callbackRef1.set(callback); + // Do not consume the data to reduce the session window. + if (frame.isEndStream()) + prepareLatch.countDown(); } }); + Assert.assertTrue(prepareLatch.await(5, TimeUnit.SECONDS)); - // Second request will consume the session window, which is now stalled. - // A third request will not be able to receive data. + final AtomicReference callbackRef2 = new AtomicReference<>(); + final AtomicReference callbackRef3 = new AtomicReference<>(); + + // Second request will consume half the session window. MetaData.Request request2 = newRequest("GET", new HttpFields()); session.newStream(new HeadersFrame(0, request2, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() { @@ -392,10 +394,23 @@ public class FlowControlTest extends AbstractTest } }); - // Third request is now stalled. - final CountDownLatch latch = new CountDownLatch(1); + // Third request will consume the session window, which is now stalled. + // A fourth request will not be able to receive data. MetaData.Request request3 = newRequest("GET", new HttpFields()); session.newStream(new HeadersFrame(0, request3, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame frame, Callback callback) + { + // Do not consume it to stall flow control. + callbackRef3.set(callback); + } + }); + + // Fourth request is now stalled. + final CountDownLatch latch = new CountDownLatch(1); + MetaData.Request request4 = newRequest("GET", new HttpFields()); + session.newStream(new HeadersFrame(0, request4, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -409,9 +424,9 @@ public class FlowControlTest extends AbstractTest // Verify that the data does not arrive because the server session is stalled. Assert.assertFalse(latch.await(1, TimeUnit.SECONDS)); - // Consume the data of the second response. + // Consume the data of the third response. // This will open up the session window, allowing the third stream to send data. - Callback callback2 = callbackRef2.getAndSet(null); + Callback callback2 = callbackRef3.getAndSet(null); if (callback2 != null) callback2.succeeded(); diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/FlowControl.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/FlowControl.java index d4087435b33..4d3f681003e 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/FlowControl.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/FlowControl.java @@ -22,6 +22,8 @@ import org.eclipse.jetty.http2.frames.WindowUpdateFrame; public interface FlowControl { + public static int DEFAULT_WINDOW_SIZE = 65535; + public void onNewStream(IStream stream); public int getInitialWindowSize(); diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2FlowControl.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2FlowControl.java index 8f69833c527..91cc53a640a 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2FlowControl.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2FlowControl.java @@ -54,10 +54,7 @@ public class HTTP2FlowControl implements FlowControl this.initialWindowSize = initialWindowSize; int delta = initialWindowSize - windowSize; - // Update the session's window size. - session.onUpdateWindowSize(null, new WindowUpdateFrame(0, delta)); - - // Update the streams' window size. + // SPEC: updates of the initial window size only affect stream windows, not session's. for (Stream stream : session.getStreams()) session.onUpdateWindowSize((IStream)stream, new WindowUpdateFrame(stream.getId(), delta)); } 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 1852df83fb6..4148a70e453 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 @@ -213,7 +213,7 @@ public abstract class HTTP2Session implements ISession, Parser.Listener if (settings.containsKey(SettingsFrame.MAX_FRAME_SIZE)) { int maxFrameSize = settings.get(SettingsFrame.MAX_FRAME_SIZE); - // Spec: check the max frame size is sane. + // SPEC: check the max frame size is sane. if (maxFrameSize < Frame.DEFAULT_MAX_LENGTH || maxFrameSize > Frame.MAX_MAX_LENGTH) { onConnectionFailure(ErrorCodes.PROTOCOL_ERROR, "invalid_settings_max_frame_size"); diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java index 17eba76fd0a..cb238babfd9 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java @@ -20,6 +20,7 @@ package org.eclipse.jetty.http2.server; import java.util.concurrent.Executor; +import org.eclipse.jetty.http2.FlowControl; import org.eclipse.jetty.http2.HTTP2Connection; import org.eclipse.jetty.http2.HTTP2FlowControl; import org.eclipse.jetty.http2.ISession; @@ -36,7 +37,7 @@ import org.eclipse.jetty.server.Connector; public abstract class AbstractHTTP2ServerConnectionFactory extends AbstractConnectionFactory { private int maxHeaderTableSize = 4096; - private int initialWindowSize = 65535; + private int initialWindowSize = FlowControl.DEFAULT_WINDOW_SIZE; private int maxConcurrentStreams = -1; public AbstractHTTP2ServerConnectionFactory()