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: [email protected]
For additional commands, e-mail: [email protected]
