andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Apply HTTP/2 local settings after ACK is received

Browse Source 
Motivation:

Previously local settings is applied on its transmission.  But this
makes settings value out-of-sync with remote endpoint.  The
application of local settings must be done after the remote endpoint
sends back its SETTINGS ACK.  This synchronization is vital to the
protocol.  Suppose that server sends SETTINGS_HEADER_TABLE_SIZE 0 on
connection establishment and resizes its header table size down to 0.
Meanwhile, client sends HEADERS which has header block encoded in
default 4096 header table size, because client has not seen the
SETTINGS from server.  Server tries to decode received HEADERS but due
to the difference of header table size, decoding process may fail.

Modifications:

We don't apply settings on transmission.  Instead, we keep track of
outstanding settings in FIFO queue.  When we get ACK, we pop oldest
outstanding settings and apply its values.

Result:

The outstanding settings values are applied when its ACK is received,
which is compliant behavior to the HTTP/2 specification.
This commit is contained in:
Tatsuhiro Tsujikawa 2014-05-09 23:37:47 +09:00 committed by Norman Maurer
parent e22aed284b
commit f978999d6a
2 changed files with 50 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandler.java
View File

@ -35,6 +35,7 @@ import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelPromise;
import io.netty.handler.codec.ByteToMessageDecoder;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@ -65,6 +66,9 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
private boolean initialSettingsReceived;
private ChannelHandlerContext ctx;
private ChannelFutureListener closeListener;
// We prefer ArrayDeque to LinkedList because later will produce more GC.
// This initial capacity is plenty for SETTINGS traffic.
private final ArrayDeque<Http2Settings> outstandingLocalSettingsQueue = new ArrayDeque<Http2Settings>(4);
protected AbstractHttp2ConnectionHandler(boolean server) {
this(server, false);
@ -292,6 +296,7 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
protected ChannelFuture writeSettings(ChannelHandlerContext ctx, ChannelPromise promise,
Http2Settings settings) throws Http2Exception {
outstandingLocalSettingsQueue.add(settings);
try {
if (connection.isGoAway()) {
throw protocolError("Sending settings after connection going away.");
@ -301,23 +306,6 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
if (connection.isServer()) {
throw protocolError("Server sending SETTINGS frame with ENABLE_PUSH specified");
}
connection.local().allowPushTo(settings.pushEnabled());
}
if (settings.hasAllowCompressedData()) {
connection.local().allowCompressedData(settings.allowCompressedData());
}
if (settings.hasMaxConcurrentStreams()) {
connection.remote().maxStreams(settings.maxConcurrentStreams());
}
if (settings.hasMaxHeaderTableSize()) {
frameReader.maxHeaderTableSize(settings.maxHeaderTableSize());
}
if (settings.hasInitialWindowSize()) {
inboundFlow.initialInboundWindowSize(settings.initialWindowSize());
}
return frameWriter.writeSettings(ctx, promise, settings);
@ -511,7 +499,11 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
private void sendInitialSettings(final ChannelHandlerContext ctx) throws Http2Exception {
if (!initialSettingsSent && ctx.channel().isActive()) {
initialSettingsSent = true;
frameWriter.writeSettings(ctx, ctx.newPromise(), settings()).addListener(
Http2Settings settings = settings();
outstandingLocalSettingsQueue.add(settings);
frameWriter.writeSettings(ctx, ctx.newPromise(), settings).addListener(
ChannelFutureListener.CLOSE_ON_FAILURE);
}
}
@ -659,6 +651,31 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
@Override
public void onSettingsAckRead(ChannelHandlerContext ctx) throws Http2Exception {
verifyInitialSettingsReceived();
// Apply oldest outstanding local settings here. This is a synchronization point
// between endpoints.
Http2Settings settings = outstandingLocalSettingsQueue.poll();
if (settings != null) {
if (settings.hasPushEnabled()) {
connection.local().allowPushTo(settings.pushEnabled());
}
if (settings.hasAllowCompressedData()) {
connection.local().allowCompressedData(settings.allowCompressedData());
}
if (settings.hasMaxConcurrentStreams()) {
connection.remote().maxStreams(settings.maxConcurrentStreams());
}
if (settings.hasMaxHeaderTableSize()) {
frameReader.maxHeaderTableSize(settings.maxHeaderTableSize());
}
if (settings.hasInitialWindowSize()) {
inboundFlow.initialInboundWindowSize(settings.initialWindowSize());
}
}
AbstractHttp2ConnectionHandler.this.onSettingsAckRead(ctx);
}

18
codec-http2/src/test/java/io/netty/handler/codec/http2/DelegatingHttp2ConnectionHandlerTest.java
View File

@ -153,6 +153,9 @@ public class DelegatingHttp2ConnectionHandlerTest {
verify(observer).onSettingsRead(eq(ctx), eq(new Http2Settings()));
verify(writer).writeSettingsAck(eq(ctx), eq(promise));
// Simulate receiving the SETTINGS ACK for the initial settings.
decode().onSettingsAckRead(ctx);
// Re-mock the context so no calls are registered.
mockContext();
handler.handlerAdded(ctx);
@ -393,7 +396,8 @@ public class DelegatingHttp2ConnectionHandlerTest {
@Test
public void settingsReadWithAckShouldNotifyObserver() throws Exception {
decode().onSettingsAckRead(ctx);
verify(observer).onSettingsAckRead(eq(ctx));
// Take into account the time this was called during setup().
verify(observer, times(2)).onSettingsAckRead(eq(ctx));
}
@Test(expected = Http2Exception.class)
@ -585,7 +589,7 @@ public class DelegatingHttp2ConnectionHandlerTest {
}
@Test
public void settingsWriteShouldUpdateSettings() throws Exception {
public void settingsWriteShouldNotUpdateSettings() throws Exception {
Http2Settings settings = new Http2Settings();
settings.allowCompressedData(false);
settings.initialWindowSize(100);
@ -594,11 +598,19 @@ public class DelegatingHttp2ConnectionHandlerTest {
settings.maxHeaderTableSize(2000);
handler.writeSettings(ctx, promise, settings);
verify(writer).writeSettings(eq(ctx), eq(promise), eq(settings));
// Verify that application of local settings must not be done when it is dispatched.
verify(local, never()).allowCompressedData(eq(false));
verify(inboundFlow, never()).initialInboundWindowSize(eq(100));
verify(local, never()).allowPushTo(eq(false));
verify(remote, never()).maxStreams(eq(1000));
verify(reader, never()).maxHeaderTableSize(eq(2000));
// Verify that settings values are applied on the reception of SETTINGS ACK
decode().onSettingsAckRead(ctx);
verify(local).allowCompressedData(eq(false));
verify(inboundFlow).initialInboundWindowSize(eq(100));
verify(local).allowPushTo(eq(false));
verify(remote).maxStreams(eq(1000));
verify(reader).maxHeaderTableSize(2000);
verify(reader).maxHeaderTableSize(eq(2000));
}
@Test(expected = Http2Exception.class)