Author: markt Date: Fri Jun 5 18:59:09 2015 New Revision: 1683844 URL: http://svn.apache.org/r1683844 Log: Clarify receiveEndOfStream vs sendEndOfStream Correct comment re streamId for initial HTTP upgrade Refactor the handling of the switch from HTTP/1.1 to HTTP/2 (still not completely happy with this) Refactor StreamStateMachine - add debug logging for state changes - remove checks for changes - these are handled by checkFrameType (considering more changes here too) Use -1 as the connection ID for the test client to make debug logs easier to read Add another test for section 5.1
Modified: tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties tomcat/trunk/java/org/apache/coyote/http2/Stream.java tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_5_1.java Modified: tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java (original) +++ tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java Fri Jun 5 18:59:09 2015 @@ -138,13 +138,13 @@ class Http2Parser { if (dest == null) { swallow(payloadSize); if (endOfStream) { - output.endOfStream(streamId); + output.receiveEndOfStream(streamId); } } else { synchronized (dest) { input.fill(true, dest, payloadSize); if (endOfStream) { - output.endOfStream(streamId); + output.receiveEndOfStream(streamId); } dest.notifyAll(); } @@ -200,7 +200,7 @@ class Http2Parser { if (Flags.isEndOfStream(flags)) { if (headersCurrentStream == -1) { - output.endOfStream(streamId); + output.receiveEndOfStream(streamId); } else { headersEndStream = true; } @@ -318,7 +318,7 @@ class Http2Parser { output.headersEnd(streamId); headersCurrentStream = -1; if (headersEndStream) { - output.endOfStream(streamId); + output.receiveEndOfStream(streamId); headersEndStream = false; } } @@ -494,7 +494,7 @@ class Http2Parser { // Data frames ByteBuffer getInputByteBuffer(int streamId, int payloadSize) throws Http2Exception; - void endOfStream(int streamId); + void receiveEndOfStream(int streamId); // Header frames HeaderEmitter headersStart(int streamId) throws Http2Exception; Modified: tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java (original) +++ tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java Fri Jun 5 18:59:09 2015 @@ -131,7 +131,7 @@ public class Http2UpgradeHandler extends this.adapter = adapter; this.connectionId = Integer.toString(connectionIdGenerator.getAndIncrement()); - // Initial HTTP request becomes stream 0. + // Initial HTTP request becomes stream 1. if (coyoteRequest != null) { Integer key = Integer.valueOf(1); Stream stream = new Stream(key, this, coyoteRequest); @@ -423,6 +423,7 @@ public class Http2UpgradeHandler extends header[3] = FrameType.DATA.getIdByte(); if (stream.getOutputBuffer().isFinished()) { header[4] = FLAG_END_OF_STREAM; + stream.sendEndOfStream(); } ByteUtil.set31Bits(header, 5, stream.getIdentifier().intValue()); socketWrapper.write(true, header, 0, header.length); @@ -716,10 +717,10 @@ public class Http2UpgradeHandler extends @Override - public void endOfStream(int streamId) { + public void receiveEndOfStream(int streamId) { Stream stream = getStream(streamId); if (stream != null) { - stream.setEndOfStream(); + stream.receiveEndOfStream(); } } Modified: tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties (original) +++ tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties Fri Jun 5 18:59:09 2015 @@ -61,8 +61,8 @@ stream.write=Connection [{0}], Stream [{ streamProcessor.httpupgrade.notsupported=HTTP upgrade is not supported within HTTP/2 streams -streamStateMachine.invalidFrame.windowUpdate=Connection [{0}], Received Data frame for stream [{1}] in state [{2}] -streamStateMachine.invalidFrame.windowUpdate=Connection [{0}], Received Window Update frame for stream [{1}] in state [{2}] +streamStateMachine.debug.change=Connection [{0}], Stream [{1}], State changed from [{2}] to [{3}] +streamStateMachine.invalidFrame=Connection [{0}], Stream [{1}], State [{2}], Frame type [{3}] upgradeHandler.connectionError=An error occurred that requires the HTTP/2 connection to be closed. upgradeHandler.goaway.debug=Connection [{0}], Goaway, Last stream [{1}], Error code [{2}], Debug data [{3}] Modified: tomcat/trunk/java/org/apache/coyote/http2/Stream.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Stream.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http2/Stream.java (original) +++ tomcat/trunk/java/org/apache/coyote/http2/Stream.java Fri Jun 5 18:59:09 2015 @@ -40,13 +40,13 @@ public class Stream extends AbstractStre private final Http2UpgradeHandler handler; private final Request coyoteRequest; private final Response coyoteResponse = new Response(); - private final StreamInputBuffer inputBuffer = new StreamInputBuffer(); + private final StreamInputBuffer inputBuffer; private final StreamOutputBuffer outputBuffer = new StreamOutputBuffer(); private final StreamStateMachine state; public Stream(Integer identifier, Http2UpgradeHandler handler) { - this(identifier, handler, new Request()); + this(identifier, handler, null); } @@ -56,15 +56,22 @@ public class Stream extends AbstractStre setParentStream(handler); setWindowSize(handler.getRemoteSettings().getInitialWindowSize()); state = new StreamStateMachine(this); - this.coyoteRequest = coyoteRequest; - this.coyoteRequest.setInputBuffer(inputBuffer); - this.coyoteResponse.setOutputBuffer(outputBuffer); - this.coyoteRequest.setResponse(coyoteResponse); - if (coyoteRequest.isFinished()) { - // Update the state machine + if (coyoteRequest == null) { + // HTTP/2 new request + this.coyoteRequest = new Request(); + this.inputBuffer = new StreamInputBuffer(); + this.coyoteRequest.setInputBuffer(inputBuffer); + } else { + // HTTP/1.1 upgrade + this.coyoteRequest = coyoteRequest; + this.inputBuffer = null; + // Headers have been populated by this point state.receiveHeaders(); + // TODO Assuming the body has been read at this point is not valid state.recieveEndOfStream(); } + this.coyoteResponse.setOutputBuffer(outputBuffer); + this.coyoteRequest.setResponse(coyoteResponse); } @@ -101,7 +108,7 @@ public class Stream extends AbstractStre log.debug(sm.getString("stream.reset.debug", getConnectionId(), getIdentifier(), Long.toString(errorCode))); } - state.recieveReset(); + state.receiveReset(); } @@ -233,10 +240,16 @@ public class Stream extends AbstractStre } - void setEndOfStream() { + void receiveEndOfStream() { state.recieveEndOfStream(); } + + void sendEndOfStream() { + state.sendEndOfStream(); + } + + StreamOutputBuffer getOutputBuffer() { return outputBuffer; } Modified: tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java (original) +++ tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java Fri Jun 5 18:59:09 2015 @@ -19,6 +19,8 @@ package org.apache.coyote.http2; import java.util.HashSet; import java.util.Set; +import org.apache.juli.logging.Log; +import org.apache.juli.logging.LogFactory; import org.apache.tomcat.util.res.StringManager; /** @@ -33,107 +35,71 @@ import org.apache.tomcat.util.res.String */ public class StreamStateMachine { + private static final Log log = LogFactory.getLog(StreamStateMachine.class); private static final StringManager sm = StringManager.getManager(StreamStateMachine.class); private final Stream stream; - private State state = State.IDLE; + private State state; public StreamStateMachine(Stream stream) { this.stream = stream; + stateChange(null, State.IDLE); } public synchronized void sendPushPromise() { - if (state == State.IDLE) { - state = State.RESERVED_LOCAL; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.IDLE, State.RESERVED_LOCAL); } public synchronized void receivePushPromis() { - if (state == State.IDLE) { - state = State.RESERVED_REMOTE; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.IDLE, State.RESERVED_REMOTE); } public synchronized void sendHeaders() { - if (state == State.IDLE) { - state = State.OPEN; - } else if (state == State.RESERVED_LOCAL) { - state = State.HALF_CLOSED_REMOTE; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.IDLE, State.OPEN); + stateChange(State.RESERVED_LOCAL, State.HALF_CLOSED_REMOTE); } public synchronized void receiveHeaders() { - if (state == State.IDLE) { - state = State.OPEN; - } else if (state == State.RESERVED_REMOTE) { - state = State.HALF_CLOSED_LOCAL; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.IDLE, State.OPEN); + stateChange(State.RESERVED_REMOTE, State.HALF_CLOSED_LOCAL); } public synchronized void sendEndOfStream() { - if (state == State.OPEN) { - state = State.HALF_CLOSED_LOCAL; - } else if (state == State.HALF_CLOSED_REMOTE) { - state = State.CLOSED; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.OPEN, State.HALF_CLOSED_LOCAL); + stateChange(State.HALF_CLOSED_REMOTE, State.CLOSED_TX); } public synchronized void recieveEndOfStream() { - if (state == State.OPEN) { - state = State.HALF_CLOSED_REMOTE; - } else if (state == State.HALF_CLOSED_LOCAL) { - state = State.CLOSED; - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + stateChange(State.OPEN, State.HALF_CLOSED_REMOTE); + stateChange(State.HALF_CLOSED_LOCAL, State.CLOSED_RX); } - public synchronized void sendReset() { - state = State.CLOSED; + private void stateChange(State oldState, State newState) { + if (state == oldState) { + state = newState; + if (log.isDebugEnabled()) { + log.debug(sm.getString("streamStateMachine.debug.change", stream.getConnectionId(), + stream.getIdentifier(), oldState, newState)); + } + } } - public synchronized void recieveReset() { - state = State.CLOSED_RESET; + public synchronized void sendReset() { + state = State.CLOSED_TX; } - public synchronized void receiveHeader() { - // Doesn't change state (that happens at the end of the headers when - // receiveHeaders() is called. This just checks that the stream is in a - // valid state to receive headers. - if (state == State.CLOSED_RESET) { - // Allow this. Client may not know that stream has been reset. - } else if (state == State.IDLE || state == State.RESERVED_REMOTE) { - // Allow these. This is normal operation. - } else { - // TODO: ProtocolExcpetion? i18n - throw new IllegalStateException(); - } + public synchronized void receiveReset() { + state = State.CLOSED_RST; } @@ -141,9 +107,15 @@ public class StreamStateMachine { // No state change. Checks that the frame type is valid for the current // state of this stream. if (!isFrameTypePermitted(frameType)) { + int errorStream; + if (state.connectionErrorForInvalidFrame) { + errorStream = 0; + } else { + errorStream = stream.getIdentifier().intValue(); + } throw new Http2Exception(sm.getString("streamStateMachine.invalidFrame", stream.getConnectionId(), stream.getIdentifier(), state, frameType), - 0, state.errorCodeForInvalidFrame); + errorStream, state.errorCodeForInvalidFrame); } } @@ -154,27 +126,31 @@ public class StreamStateMachine { private enum State { - IDLE (ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY), - OPEN (ErrorCode.PROTOCOL_ERROR, FrameType.DATA, FrameType.HEADERS, + IDLE (true, ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY), + OPEN (true, ErrorCode.PROTOCOL_ERROR, FrameType.DATA, FrameType.HEADERS, FrameType.PRIORITY, FrameType.RST, FrameType.PUSH_PROMISE, FrameType.WINDOW_UPDATE), - RESERVED_LOCAL (ErrorCode.PROTOCOL_ERROR, FrameType.PRIORITY, FrameType.RST, + RESERVED_LOCAL (true, ErrorCode.PROTOCOL_ERROR, FrameType.PRIORITY, FrameType.RST, FrameType.WINDOW_UPDATE), - RESERVED_REMOTE (ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY, + RESERVED_REMOTE (true, ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY, FrameType.RST), - HALF_CLOSED_LOCAL (ErrorCode.PROTOCOL_ERROR, FrameType.DATA, FrameType.HEADERS, + HALF_CLOSED_LOCAL (true, ErrorCode.PROTOCOL_ERROR, FrameType.DATA, FrameType.HEADERS, FrameType.PRIORITY, FrameType.RST, FrameType.PUSH_PROMISE, FrameType.WINDOW_UPDATE), - HALF_CLOSED_REMOTE (ErrorCode.STREAM_CLOSED, FrameType.PRIORITY, FrameType.RST, - FrameType.WINDOW_UPDATE), - CLOSED (ErrorCode.PROTOCOL_ERROR, FrameType.PRIORITY, FrameType.RST, + HALF_CLOSED_REMOTE (true, ErrorCode.STREAM_CLOSED, FrameType.PRIORITY, FrameType.RST, FrameType.WINDOW_UPDATE), - CLOSED_RESET (ErrorCode.PROTOCOL_ERROR, FrameType.PRIORITY); + CLOSED_RX (true, ErrorCode.STREAM_CLOSED, FrameType.PRIORITY), + CLOSED_RST (false, ErrorCode.STREAM_CLOSED, FrameType.PRIORITY), + CLOSED_TX (true, ErrorCode.STREAM_CLOSED, FrameType.PRIORITY, FrameType.RST, + FrameType.WINDOW_UPDATE); + private final boolean connectionErrorForInvalidFrame; private final ErrorCode errorCodeForInvalidFrame; private final Set<FrameType> frameTypesPermitted = new HashSet<>(); - private State(ErrorCode errorCode, FrameType... frameTypes) { + private State(boolean connectionErrorForInvalidFrame, ErrorCode errorCode, + FrameType... frameTypes) { + this.connectionErrorForInvalidFrame = connectionErrorForInvalidFrame; this.errorCodeForInvalidFrame = errorCode; for (FrameType frameType : frameTypes) { frameTypesPermitted.add(frameType); Modified: tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java (original) +++ tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java Fri Jun 5 18:59:09 2015 @@ -236,7 +236,7 @@ public abstract class Http2TestBase exte input = new TestInput(is); output = new TestOutput(); - parser = new Http2Parser("0", input, output); + parser = new Http2Parser("-1", input, output); } @@ -442,7 +442,7 @@ public abstract class Http2TestBase exte @Override - public void endOfStream(int streamId) { + public void receiveEndOfStream(int streamId) { lastStreamId = Integer.toString(streamId); trace.append(lastStreamId + "-EndOfStream\n"); } Modified: tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_5_1.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_5_1.java?rev=1683844&r1=1683843&r2=1683844&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_5_1.java (original) +++ tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_5_1.java Fri Jun 5 18:59:09 2015 @@ -73,7 +73,20 @@ public class TestHttp2Section_5_1 extend // This should trigger a stream error sendData(3, new byte[] {}); + parser.readFrame(true); + + Assert.assertTrue(output.getTrace(), + output.getTrace().startsWith("0-Goaway-[2147483647]-[" + + ErrorCode.STREAM_CLOSED.getErrorCode() + "]-[")); + } + + + @Test + public void testClosedInvalidFrame() throws Exception { + http2Connect(); + // Stream 1 is closed. This should trigger a stream error + sendData(1, new byte[] {}); parser.readFrame(true); Assert.assertTrue(output.getTrace(), --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org