diff --git a/java/org/apache/coyote/http2/LocalStrings.properties b/java/org/apache/coyote/http2/LocalStrings.properties
index 48c7ab33f645..e1fd3528787f 100644
--- 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
index b2a36fc2e289..c8596933be01 100644
--- 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
index fe5bb2ade091..caa3debea35a 100644
--- 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
index 372483929994..a2b6ee452f00 100644
--- 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)
+
+ 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)
+