[#3724] HTTP/2 Headers END_STREAM results in RST_STREAM

Motivation:
If headers are sent on a stream that does not yet exist and the END_STREAM flag is set we will send a RST_STREAM frame. We should send the HEADERS frame and no RST_STREAM.

Modifications:
DefaultHttp2RemoteFlowController should allow frames to be sent if stream is created in the 'half closed (local)' state.

Result:
We can send HEADERS frame with the END_STREAM flag sent without sending a RST_STREAM frame.
This commit is contained in:
Scott Mitchell 2015-05-04 15:15:44 -07:00
parent bace371ca5
commit 74627483d7
6 changed files with 45 additions and 13 deletions

View File

@ -434,9 +434,6 @@ public class DefaultHttp2Connection implements Http2Connection {
public Http2Stream open(boolean halfClosed) throws Http2Exception {
state = activeState(id, state, isLocal(), halfClosed);
activate();
if (halfClosed) {
notifyHalfClosed(this);
}
return this;
}
@ -900,9 +897,6 @@ public class DefaultHttp2Connection implements Http2Connection {
public DefaultStream createStream(int streamId, boolean halfClosed) throws Http2Exception {
DefaultStream stream = createStream(streamId, activeState(streamId, IDLE, isLocal(), halfClosed));
stream.activate();
if (halfClosed) {
notifyHalfClosed(stream);
}
return stream;
}

View File

@ -529,7 +529,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
/**
* Helper method for determining whether or not to ignore inbound frames. A stream is considered to be created
* after a go away is sent if the following conditions hold:
* after a {@code GOAWAY} is sent if the following conditions hold:
* <p/>
* <ul>
* <li>A {@code GOAWAY} must have been sent by the local endpoint</li>

View File

@ -605,8 +605,7 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
frame.writeComplete();
}
} catch (Throwable t) {
// Mark the state as cancelled, we'll clear the pending queue
// via cancel() below.
// Mark the state as cancelled, we'll clear the pending queue via cancel() below.
cancelled = true;
cause = t;
} finally {

View File

@ -43,8 +43,10 @@ public interface Http2Connection {
void onStreamActive(Http2Stream stream);
/**
* Notifies the listener that the given stream is now {@code HALF CLOSED}. The stream can be
* inspected to determine which side is {@code CLOSED}.
* Notifies the listener that the given stream has transitioned from {@code OPEN} to {@code HALF CLOSED}.
* This method will <strong>not</strong> be called until a state transition occurs from when
* {@link #onStreamActive(Http2Stream)} was called.
* The stream can be inspected to determine which side is {@code HALF CLOSED}.
* <p>
* If a {@link RuntimeException} is thrown it will be logged and <strong>not propagated</strong>.
* Throwing from this method is not supported and is considered a programming error.

View File

@ -42,6 +42,7 @@ import io.netty.channel.socket.nio.NioServerSocketChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpHeaderValues;
import io.netty.handler.codec.http2.Http2Stream.State;
import io.netty.handler.codec.http2.Http2TestUtil.FrameAdapter;
import io.netty.handler.codec.http2.Http2TestUtil.FrameCountDown;
import io.netty.handler.codec.http2.Http2TestUtil.Http2Runnable;
@ -270,6 +271,12 @@ public class DataCompressionHttp2Test {
clientConnection = new DefaultHttp2Connection(false);
serverConnection.addListener(new Http2ConnectionAdapter() {
@Override
public void onStreamActive(Http2Stream stream) {
if (stream.state() == State.HALF_CLOSED_LOCAL || stream.state() == State.HALF_CLOSED_REMOTE) {
serverLatch.countDown();
}
}
@Override
public void onStreamHalfClosed(Http2Stream stream) {
serverLatch.countDown();

View File

@ -30,6 +30,7 @@ import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
@ -110,9 +111,38 @@ public class Http2ConnectionRoundtripTest {
clientGroup.sync();
}
@Test
public void headersWithEndStreamShouldNotSendError() throws Exception {
bootstrapEnv(1, 1, 2, 1);
// Create a single stream by sending a HEADERS frame to the server.
final short weight = 16;
final Http2Headers headers = dummyHeaders();
runInChannel(clientChannel, new Http2Runnable() {
@Override
public void run() {
http2Client.encoder().writeHeaders(ctx(), 3, headers, 0, weight, false, 0, true,
newPromise());
ctx().flush();
}
});
assertTrue(requestLatch.await(5, SECONDS));
verify(serverListener).onHeadersRead(any(ChannelHandlerContext.class), eq(3), eq(headers),
eq(0), eq(weight), eq(false), eq(0), eq(true));
// Wait for some time to see if a go_away or reset frame will be received.
Thread.sleep(1000);
// Verify that no errors have been received.
verify(serverListener, never()).onGoAwayRead(any(ChannelHandlerContext.class), anyInt(),
anyLong(), any(ByteBuf.class));
verify(serverListener, never()).onRstStreamRead(any(ChannelHandlerContext.class), anyInt(),
anyLong());
}
@Test
public void http2ExceptionInPipelineShouldCloseConnection() throws Exception {
bootstrapEnv(1, 1, 1, 1);
bootstrapEnv(1, 1, 2, 1);
// Create a latch to track when the close occurs.
final CountDownLatch closeLatch = new CountDownLatch(1);
@ -190,7 +220,7 @@ public class Http2ConnectionRoundtripTest {
@Test
public void nonHttp2ExceptionInPipelineShouldNotCloseConnection() throws Exception {
bootstrapEnv(1, 1, 1, 1);
bootstrapEnv(1, 1, 2, 1);
// Create a latch to track when the close occurs.
final CountDownLatch closeLatch = new CountDownLatch(1);