Split Stream.recycle() into replace() and recycle()

Ensures recycle() is called no more than once. There are some error conditions that now no longer call recycle() to avoid the risk of duplicate calls to recycle if a Stream is being processed by a StreamProcessor at the same time Http2UpgradeHandler detects a Stream level error.
apache · Oct 2, 2024 · 146f94f · 146f94f
1 parent f7057dd
commit 146f94f
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 15 deletions.
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -109,7 +109,8 @@ stream.inputBuffer.signal=Data added to inBuffer when read thread is waiting. Si
 stream.inputBuffer.swallowUnread=Swallowing [{0}] bytes previously read into input stream buffer
 stream.notWritable=Connection [{0}], Stream [{1}], This stream is not writable
 stream.outputBuffer.flush.debug=Connection [{0}], Stream [{1}], flushing output with buffer at position [{2}], writeInProgress [{3}] and closed [{4}]
-stream.recycle=Connection [{0}], Stream [{1}] has been recycled
+stream.recycle.duplicate=Connection [{0}], Stream [{1}] Duplicate request to recycle the associated request and response has been ignored
+stream.recycle.first=Connection [{0}], Stream [{1}] The associated request and response have been recycled
 stream.reset.fail=Connection [{0}], Stream [{1}], Failed to reset stream
 stream.reset.receive=Connection [{0}], Stream [{1}], Reset received due to [{2}]
 stream.reset.send=Connection [{0}], Stream [{1}], Reset sent due to [{2}]

diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
@@ -109,6 +109,9 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
     private volatile int urgency = Priority.DEFAULT_URGENCY;
     private volatile boolean incremental = Priority.DEFAULT_INCREMENTAL;
 
+    private final Object recycledLock = new Object();
+    private volatile boolean recycled = false;
+
 
     Stream(Integer identifier, Http2UpgradeHandler handler) {
         this(identifier, handler, null);
@@ -784,20 +787,15 @@ final void close(Http2Exception http2Exception) {
         } else {
             handler.closeConnection(http2Exception);
         }
-        recycle();
+        replace();
     }
 
 
     /*
-     * This method is called recycle for consistency with the rest of the Tomcat code base. Currently, it calls the
-     * handler to replace this stream with an implementation that uses less memory. It does not fully recycle the Stream
-     * ready for re-use since Stream objects are not re-used. This is useful because Stream instances are retained for a
-     * period after the Stream closes.
+     * This method calls the handler to replace this stream with an implementation that uses less memory. This is useful
+     * because Stream instances are retained for a period after the Stream closes.
      */
-    final void recycle() {
-        if (log.isTraceEnabled()) {
-            log.trace(sm.getString("stream.recycle", getConnectionId(), getIdAsString()));
-        }
+    final void replace() {
         int remaining;
         // May be null if stream was closed before any DATA frames were processed.
         ByteBuffer inputByteBuffer = getInputByteBuffer(false);
@@ -807,6 +805,40 @@ final void recycle() {
             remaining = inputByteBuffer.remaining();
         }
         handler.replaceStream(this, new RecycledStream(getConnectionId(), getIdentifier(), state, remaining));
+    }
+
+
+    /*
+     * This method is called recycle for consistency with the rest of the Tomcat code base. It does not recycle the
+     * Stream since Stream objects are not re-used. It does recycle the request and response objects and ensures that
+     * this is only done once.
+     *
+     * replace() should have been called before calling this method.
+     *
+     * It is important that this method is not called until any concurrent processing for the stream has completed. This
+     * is currently achieved by:
+     * - only the StreamProcessor calls this method
+     * - the Http2UpgradeHandler does not call this method
+     * - this method is called once the StreamProcessor considers the Stream closed
+     *
+     * In theory, the protection against duplicate calls is not required in this method (the code in StreamProcessor
+     * should be sufficient) but it is implemented as precaution along with the WARN level logging.
+     */
+    final void recycle() {
+        if (recycled) {
+            log.warn(sm.getString("stream.recycle.duplicate", getConnectionId(), getIdAsString()));
+            return;
+        }
+        synchronized (recycledLock) {
+            if (recycled) {
+                log.warn(sm.getString("stream.recycle.duplicate", getConnectionId(), getIdAsString()));
+                return;
+            }
+            recycled = true;
+        }
+        if (log.isTraceEnabled()) {
+            log.trace(sm.getString("stream.recycle.first", getConnectionId(), getIdAsString()));
+        }
         coyoteRequest.recycle();
         coyoteResponse.recycle();
         handler.getProtocol().pushRequestAndResponse(coyoteRequest);

diff --git a/java/org/apache/coyote/http2/StreamProcessor.java b/java/org/apache/coyote/http2/StreamProcessor.java
@@ -87,9 +87,9 @@ final void process(SocketEvent event) {
             try {
                 /*
                  * In some scenarios, error handling may trigger multiple ERROR events for the same stream. The first
-                 * ERROR event process will close the stream and recycle it. Once the stream has been recycled it should
-                 * not be used for processing any further events. The check below ensures that this is the case. In
-                 * particular, Stream.recycle() should not be called more than once per Stream.
+                 * ERROR event processed will close the stream, replace it and recycle it. Once the stream has been
+                 * replaced it should not be used for processing any further events. When it is known that processing is
+                 * going to be a NO-OP, exit early.
                  */
                 if (!stream.equals(handler.getStream(stream.getIdAsInt()))) {
                     return;
@@ -130,8 +130,8 @@ final void process(SocketEvent event) {
                             stream.close(se);
                         } else {
                             if (!stream.isActive()) {
-                                // stream.close() will call recycle so only need it here
-                                stream.recycle();
+                                // Close calls replace() so need the same call here
+                                stream.replace();
                             }
                         }
                     }
@@ -146,6 +146,7 @@ final void process(SocketEvent event) {
                     state = SocketState.CLOSED;
                 } finally {
                     if (state == SocketState.CLOSED) {
+                        stream.recycle();
                         recycle();
                     }
                 }

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -173,6 +173,12 @@
         stream nor are trailer fields for an in progress stream swallowed if the
         Connector is paused before the trailer fields are received. (markt)
       </fix>
+      <fix>
+        Ensure the request and response are not recycled too soon for an HTTP/2
+        stream when a stream level error is detected during the processing of
+        incoming HTTP/2 frames. This could lead to incorrect processing times
+        appearing in the access log. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">