Change state on start of headers not end of headers

More renames to improve clarity
Fill some gaps in the debug logging
Add a (currently disabled because it fails) test that should trigger a stream close

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1683857 13f79535-47bb-0310-9956-ffa450edef68

java/org/apache/coyote/http2/Http2UpgradeHandler.java

@@ -133,6 +133,9 @@ public Http2UpgradeHandler(Adapter adapter, Request coyoteRequest) {
 
         // Initial HTTP request becomes stream 1.
         if (coyoteRequest != null) {
+            if (log.isDebugEnabled()) {
+                log.debug(sm.getString("upgradeHandler.upgrade", connectionId));
+            }
             Integer key = Integer.valueOf(1);
             Stream stream = new Stream(key, this, coyoteRequest);
             streams.put(key, stream);
@@ -238,9 +241,10 @@ public SocketState upgradeDispatch(SocketStatus status) {
                     if (log.isDebugEnabled()) {
                         log.debug(sm.getString("upgradeHandler.connectionError"), h2e);
                     }
-                    close(h2e);
+                    closeConnecion(h2e);
                     break;
                 } else {
+
                     // Stream error
                     // TODO Reset stream
                 }
@@ -263,7 +267,7 @@ public SocketState upgradeDispatch(SocketStatus status) {
                 if (h2e.getStreamId() == 0) {
                     // Connection error
                     log.warn(sm.getString("upgradeHandler.connectionError"), h2e);
-                    close(h2e);
+                    closeConnecion(h2e);
                     break;
                 } else {
                     // Stream error
@@ -335,7 +339,7 @@ public void destroy() {
     }
 
 
-    private void close(Http2Exception h2e) {
+    private void closeConnecion(Http2Exception h2e) {
         // Write a GOAWAY frame.
         byte[] fixedPayload = new byte[8];
         // TODO needs to be correct value
@@ -729,6 +733,7 @@ public void receiveEndOfStream(int streamId) {
     public HeaderEmitter headersStart(int streamId) throws Http2Exception {
         Stream stream = getStream(streamId);
         stream.checkState(FrameType.HEADERS);
+        stream.receivedStartOfHeaders();
         return stream;
     }
 
@@ -749,7 +754,6 @@ public void reprioritise(int streamId, int parentStreamId,
     @Override
     public void headersEnd(int streamId) {
         Stream stream = getStream(streamId);
-        stream.receivedEndOfHeaders();
         // Process this stream on a container thread
         StreamProcessor streamProcessor = new StreamProcessor(stream, adapter, socketWrapper);
         streamProcessor.setSslSupport(sslSupport);

java/org/apache/coyote/http2/LocalStrings.properties

@@ -64,16 +64,17 @@ streamProcessor.httpupgrade.notsupported=HTTP upgrade is not supported within HT
 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}]
 upgradeHandler.init=Connection [{0}]
 upgradeHandler.ioerror=Connection [{0}]
 upgradeHandler.sendPrefaceFail=Failed to send preface to client
 upgradeHandler.socketCloseFailed=Error closing socket
 upgradeHandler.unexpectedEos=Unexpected end of stream
 upgradeHandler.unexpectedStatus=An unexpected value of status ([{0}]) was passed to this method
+upgradeHandler.upgrade=Connection [{0}], HTTP/1.1 upgrade to stream [1]
 upgradeHandler.upgradeDispatch.entry=Entry, Connection [{0}], SocketStatus [{1}]
 upgradeHandler.upgradeDispatch.exit=Exit, Connection [{0}], SocketState [{1}]
+upgradeHandler.writeBody=Connection [{0}], Stream [{1}], Data length [{2}]
 upgradeHandler.writeHeaders=Connection [{0}], Stream [{1}]
 
 writeStateMachine.endWrite.ise=It is illegal to specify [{0}] for the new state once a write has completed

java/org/apache/coyote/http2/Stream.java

@@ -66,7 +66,7 @@ public Stream(Integer identifier, Http2UpgradeHandler handler, Request coyoteReq
             this.coyoteRequest = coyoteRequest;
             this.inputBuffer = null;
             // Headers have been populated by this point
-            state.receivedEndOfHeaders();
+            state.receivedStartOfHeaders();
             // TODO Assuming the body has been read at this point is not valid
             state.recievedEndOfStream();
         }
@@ -235,8 +235,8 @@ ByteBuffer getInputByteBuffer() {
     }
 
 
-    void receivedEndOfHeaders() {
-        state.receivedEndOfHeaders();
+    void receivedStartOfHeaders() {
+        state.receivedStartOfHeaders();
     }
 
 

java/org/apache/coyote/http2/StreamStateMachine.java

@@ -58,13 +58,13 @@ public synchronized void receivedPushPromis() {
     }
 
 
-    public synchronized void sentEndOfHeaders() {
+    public synchronized void sentStartOfHeaders() {
         stateChange(State.IDLE, State.OPEN);
         stateChange(State.RESERVED_LOCAL, State.HALF_CLOSED_REMOTE);
     }
 
 
-    public synchronized void receivedEndOfHeaders() {
+    public synchronized void receivedStartOfHeaders() {
         stateChange(State.IDLE, State.OPEN);
         stateChange(State.RESERVED_REMOTE, State.HALF_CLOSED_LOCAL);
     }
@@ -82,6 +82,16 @@ public synchronized void recievedEndOfStream() {
     }
 
 
+    public synchronized void sendReset() {
+        stateChange(state, State.CLOSED_TX);
+    }
+
+
+    public synchronized void receiveReset() {
+        stateChange(state, State.CLOSED_RST);
+    }
+
+
     private void stateChange(State oldState, State newState) {
         if (state == oldState) {
             state = newState;
@@ -93,16 +103,6 @@ private void stateChange(State oldState, State newState) {
     }
 
 
-    public synchronized void sendReset() {
-        state = State.CLOSED_TX;
-    }
-
-
-    public synchronized void receiveReset() {
-        state = State.CLOSED_RST;
-    }
-
-
     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.

test/org/apache/coyote/http2/Http2TestBase.java

@@ -355,6 +355,23 @@ void sendClientPreface() throws IOException {
     }
 
 
+    void sendRst(int streamId, long errorCode) throws IOException {
+        byte[] rstFrame = new byte[13];
+        // length is always 4
+        rstFrame[2] = 0x04;
+        // type is always 3
+        rstFrame[3] = 0x03;
+        // no flags
+        // Stream ID
+        ByteUtil.set31Bits(rstFrame, 5, streamId);
+        // Payload
+        ByteUtil.setFourBytes(rstFrame, 9, errorCode);
+
+        os.write(rstFrame);
+        os.flush();
+    }
+
+
     void sendPing() throws IOException {
         os.write(PING_FRAME);
         os.flush();

test/org/apache/coyote/http2/TestHttp2Section_5_1.java

@@ -16,7 +16,10 @@
  */
 package org.apache.coyote.http2;
 
+import java.nio.ByteBuffer;
+
 import org.junit.Assert;
+import org.junit.Ignore;
 import org.junit.Test;
 
 /**
@@ -82,7 +85,39 @@ public void halfClosedRemoteInvalidFrame() throws Exception {
 
 
     @Test
-    public void testClosedInvalidFrame() throws Exception {
+    @Ignore // Need to handle stream closes
+    public void testClosedInvalidFrame01() throws Exception {
+        hpackEncoder = new HpackEncoder(ConnectionSettings.DEFAULT_HEADER_TABLE_SIZE);
+
+        // HTTP2 upgrade
+        http2Connect();
+
+        // Build the simple request
+        byte[] frameHeader = new byte[9];
+        ByteBuffer headersPayload = ByteBuffer.allocate(128);
+        buildSimpleRequest(frameHeader, headersPayload, 3);
+
+        // Remove the end of stream and end of headers flags
+        frameHeader[4] = 0;
+
+        // Process the request
+        writeFrame(frameHeader, headersPayload);
+
+        // Send a rst
+        sendRst(3, ErrorCode.INTERNAL_ERROR.getErrorCode());
+
+        // Then try sending some data (which should fail)
+        sendData(3, new byte[] {});
+        parser.readFrame(true);
+
+        Assert.assertTrue(output.getTrace(),
+                output.getTrace().startsWith("0-Goaway-[2147483647]-[" +
+                        ErrorCode.STREAM_CLOSED.getErrorCode() + "]-["));
+    }
+
+
+    @Test
+    public void testClosedInvalidFrame02() throws Exception {
         http2Connect();
 
         // Stream 1 is closed. This should trigger a stream error