You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/06/05 12:05:35 UTC
svn commit: r1683698 - in /tomcat/trunk: java/org/apache/coyote/http2/
test/org/apache/coyote/http2/
Author: markt
Date: Fri Jun 5 10:05:35 2015
New Revision: 1683698
URL: http://svn.apache.org/r1683698
Log:
Add a test when sending a DATA frame on a stream that is already closed
Refactor the StreamStateMachine to track which frames are permitted in which states
Update the parser to check the frame type for non-zero streams
Separate incrementWindowSize (triggered by incoming frame) and decrementWindowSize(triggered by write)
Modified:
tomcat/trunk/java/org/apache/coyote/http2/AbstractStream.java
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/TestHttp2Section_5_1.java
Modified: tomcat/trunk/java/org/apache/coyote/http2/AbstractStream.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/AbstractStream.java?rev=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/AbstractStream.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/AbstractStream.java Fri Jun 5 10:05:35 2015
@@ -99,7 +99,7 @@ abstract class AbstractStream {
/**
-s * @param increment
+ * @param increment
* @throws Http2Exception
*/
protected void incrementWindowSize(int increment) throws Http2Exception {
@@ -107,6 +107,11 @@ s * @param increment
}
+ protected void decrementWindowSize(int decrement) {
+ windowSize.addAndGet(-decrement);
+ }
+
+
protected abstract String getConnectionId();
protected abstract int getWeight();
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=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java Fri Jun 5 10:05:35 2015
@@ -386,7 +386,7 @@ class Http2Parser {
if (log.isDebugEnabled()) {
log.debug(sm.getString("http2Parser.processFrame", connectionId,
- Integer.toString(streamId), Integer.toString(flags),
+ Integer.toString(streamId), frameType, Integer.toString(flags),
Integer.toString(payloadSize)));
}
@@ -497,12 +497,15 @@ class Http2Parser {
void endOfStream(int streamId);
// Header frames
- HeaderEmitter headersStart(int streamId);
- void reprioritise(int streamId, int parentStreamId, boolean exclusive, int weight);
+ HeaderEmitter headersStart(int streamId) throws Http2Exception;
void headersEnd(int streamId);
+ // Priority frames (also headers)
+ void reprioritise(int streamId, int parentStreamId, boolean exclusive, int weight)
+ throws Http2Exception;
+
// Reset frames
- void reset(int streamId, long errorCode);
+ void reset(int streamId, long errorCode) throws Http2Exception;
// Settings frames
void setting(int identifier, long value) throws IOException;
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=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Http2UpgradeHandler.java Fri Jun 5 10:05:35 2015
@@ -458,7 +458,7 @@ public class Http2UpgradeHandler extends
}
- int reserveWindowSize(Stream stream, int toWrite) throws Http2Exception {
+ int reserveWindowSize(Stream stream, int toWrite) {
int result;
synchronized (backLogLock) {
long windowSize = getWindowSize();
@@ -485,7 +485,7 @@ public class Http2UpgradeHandler extends
} else {
result = toWrite;
}
- incrementWindowSize(-result);
+ decrementWindowSize(-result);
}
return result;
}
@@ -710,6 +710,7 @@ public class Http2UpgradeHandler extends
if (stream == null) {
return null;
}
+ stream.checkState(FrameType.DATA);
return stream.getInputByteBuffer();
}
@@ -724,15 +725,18 @@ public class Http2UpgradeHandler extends
@Override
- public HeaderEmitter headersStart(int streamId) {
- return getStream(streamId);
+ public HeaderEmitter headersStart(int streamId) throws Http2Exception {
+ Stream stream = getStream(streamId);
+ stream.checkState(FrameType.HEADERS);
+ return stream;
}
@Override
public void reprioritise(int streamId, int parentStreamId,
- boolean exclusive, int weight) {
+ boolean exclusive, int weight) throws Http2Exception {
Stream stream = getStream(streamId);
+ stream.checkState(FrameType.PRIORITY);
AbstractStream parentStream = getStream(parentStreamId);
if (parentStream == null) {
parentStream = this;
@@ -754,9 +758,10 @@ public class Http2UpgradeHandler extends
@Override
- public void reset(int streamId, long errorCode) {
+ public void reset(int streamId, long errorCode) throws Http2Exception {
Stream stream = getStream(streamId);
if (stream != null) {
+ stream.checkState(FrameType.RST);
stream.reset(errorCode);
}
}
@@ -814,6 +819,9 @@ public class Http2UpgradeHandler extends
public void incrementWindowSize(int streamId, int increment) throws Http2Exception {
AbstractStream stream = getStream(streamId);
if (stream != null) {
+ if (streamId > 0) {
+ ((Stream) stream).checkState(FrameType.WINDOW_UPDATE);
+ }
stream.incrementWindowSize(increment);
}
}
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=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties Fri Jun 5 10:05:35 2015
@@ -39,7 +39,7 @@ http2Parser.headers.wrongStream=Connecti
http2Parser.payloadTooBig=The payload is [{0}] bytes long but the maximum frame size is [{1}]
http2Parser.preface.invalid=Invalid connection preface [{0}] presented
http2Parser.preface.io=Unable to read connection preface
-http2Parser.processFrame=Connection [{0}], Stream [{1}], Flags [{2}], Payload size [{3}]
+http2Parser.processFrame=Connection [{0}], Stream [{1}], Frame type [{2}], Flags [{3}], Payload size [{4}]
http2Parser.processFrame.unexpectedType=Expected frame type [{0}] but received frame type [{1}]
http2Parser.processFrameContinuation.notExpected=Connection [{0}], Continuation frame received for stream [{1}] when no headers were in progress
http2Parser.processFrameGoaway.payloadTooSmall=Connection [{0}]: Goaway payload size was [{1}] which is less than the minimum 8
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=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Stream.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Stream.java Fri Jun 5 10:05:35 2015
@@ -105,15 +105,17 @@ public class Stream extends AbstractStre
}
+ void checkState(FrameType frameType) throws Http2Exception {
+ state.checkFrameType(frameType);
+ }
+
+
@Override
public void incrementWindowSize(int windowSizeIncrement) throws Http2Exception {
// If this is zero then any thread that has been trying to write for
// this stream will be waiting. Notify that thread it can continue. Use
// notify all even though only one thread is waiting to be on the safe
// side.
- if (windowSizeIncrement > 0) {
- state.receivedWindowUpdate();
- }
boolean notify = getWindowSize() == 0;
super.incrementWindowSize(windowSizeIncrement);
if (notify) {
@@ -221,8 +223,7 @@ public class Stream extends AbstractStre
}
- ByteBuffer getInputByteBuffer() throws Http2Exception {
- state.receivedData();
+ ByteBuffer getInputByteBuffer() {
return inputBuffer.getInBuffer();
}
@@ -233,9 +234,7 @@ public class Stream extends AbstractStre
void setEndOfStream() {
- // TODO This is temporary until the state machine for a stream is
- // implemented
- inputBuffer.endOfStream = true;
+ state.recieveEndOfStream();
}
StreamOutputBuffer getOutputBuffer() {
@@ -317,7 +316,7 @@ public class Stream extends AbstractStre
}
} while (thisWrite < 1);
- incrementWindowSize(-thisWrite);
+ decrementWindowSize(thisWrite);
// Do the write
handler.writeBody(Stream.this, buffer, thisWrite);
@@ -377,8 +376,6 @@ public class Stream extends AbstractStre
// 'write mode'.
private volatile ByteBuffer inBuffer;
- private boolean endOfStream = false;
-
@Override
public int doRead(ByteChunk chunk) throws IOException {
@@ -388,7 +385,7 @@ public class Stream extends AbstractStre
// Ensure that only one thread accesses inBuffer at a time
synchronized (inBuffer) {
- while (inBuffer.position() == 0 && !endOfStream) {
+ while (inBuffer.position() == 0 && !state.isFrameTypePermitted(FrameType.DATA)) {
// Need to block until some data is written
try {
inBuffer.wait();
@@ -403,7 +400,7 @@ public class Stream extends AbstractStre
written = inBuffer.remaining();
inBuffer.get(outBuffer, 0, written);
inBuffer.clear();
- } else if (endOfStream) {
+ } else if (state.isFrameTypePermitted(FrameType.DATA)) {
return -1;
} else {
// TODO Should never happen
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=1683698&r1=1683697&r2=1683698&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/StreamStateMachine.java Fri Jun 5 10:05:35 2015
@@ -16,6 +16,9 @@
*/
package org.apache.coyote.http2;
+import java.util.HashSet;
+import java.util.Set;
+
import org.apache.tomcat.util.res.StringManager;
/**
@@ -144,52 +147,52 @@ public class StreamStateMachine {
}
- public synchronized void receivedWindowUpdate() throws Http2Exception {
- // No state change. Just checks state is valid for receiving window
- // update.
- if (!state.isWindowUpdatePermitted()) {
- throw new Http2Exception(sm.getString("streamStateMachine.invalidFrame.windowUpdate",
- stream.getConnectionId(), stream.getIdentifier(), state),
- 0, ErrorCode.PROTOCOL_ERROR);
+ public synchronized void checkFrameType(FrameType frameType) throws Http2Exception {
+ // No state change. Checks that the frame type is valid for the current
+ // state of this stream.
+ if (!isFrameTypePermitted(frameType)) {
+ throw new Http2Exception(sm.getString("streamStateMachine.invalidFrame",
+ stream.getConnectionId(), stream.getIdentifier(), state, frameType),
+ 0, state.errorCodeForInvalidFrame);
}
}
- public synchronized void receivedData() throws Http2Exception {
- // No state change. Just checks state is valid for receiving window
- // update.
- if (!state.isDataPermitted()) {
- throw new Http2Exception(sm.getString("streamStateMachine.invalidFrame.data",
- stream.getConnectionId(), stream.getIdentifier(), state),
- 0, ErrorCode.PROTOCOL_ERROR);
- }
+ public synchronized boolean isFrameTypePermitted(FrameType frameType) {
+ return state.isFrameTypePermitted(frameType);
}
private enum State {
- IDLE (false, false),
- OPEN ( true, true),
- RESERVED_LOCAL ( true, false),
- RESERVED_REMOTE (false, false),
- HALF_CLOSED_LOCAL ( true, true),
- HALF_CLOSED_REMOTE ( true, false),
- CLOSED (false, false),
- CLOSED_RESET ( true, true);
-
- private final boolean windowUpdatePermitted;
- private final boolean dataPermitted;
-
- private State(boolean windowUpdatePermitted, boolean dataPermitted) {
- this.windowUpdatePermitted = windowUpdatePermitted;
- this.dataPermitted = dataPermitted;
- }
-
- public boolean isWindowUpdatePermitted() {
- return windowUpdatePermitted;
+ IDLE (ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY),
+ OPEN (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,
+ FrameType.WINDOW_UPDATE),
+ RESERVED_REMOTE (ErrorCode.PROTOCOL_ERROR, FrameType.HEADERS, FrameType.PRIORITY,
+ FrameType.RST),
+ HALF_CLOSED_LOCAL (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,
+ FrameType.WINDOW_UPDATE),
+ CLOSED_RESET (ErrorCode.PROTOCOL_ERROR, FrameType.PRIORITY);
+
+ private final ErrorCode errorCodeForInvalidFrame;
+ private final Set<FrameType> frameTypesPermitted = new HashSet<>();
+
+ private State(ErrorCode errorCode, FrameType... frameTypes) {
+ this.errorCodeForInvalidFrame = errorCode;
+ for (FrameType frameType : frameTypes) {
+ frameTypesPermitted.add(frameType);
+ }
}
- public boolean isDataPermitted() {
- return dataPermitted;
+ public boolean isFrameTypePermitted(FrameType frameType) {
+ return frameTypesPermitted.contains(frameType);
}
}
}
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=1683698&r1=1683697&r2=1683698&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 10:05:35 2015
@@ -56,5 +56,31 @@ public class TestHttp2Section_5_1 extend
}
+ // TODO: reserved local
+ // TODO: reserved remote
+
+
+ @Test
+ public void halfClosedRemoteInvalidFrame() throws Exception {
+ hpackEncoder = new HpackEncoder(ConnectionSettings.DEFAULT_HEADER_TABLE_SIZE);
+ http2Connect();
+
+ // This half-closes the stream since it includes the end of stream flag
+ sendSimpleRequest(3);
+ readSimpleResponse();
+ Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace());
+ output.clearTrace();
+
+ // 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() + "]-["));
+ }
+
+
// TODO: Invalid frames for each of the remaining states
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org