This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 47307ee27abcdea2ee40e33897aca760083de46a
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Oct 1 18:27:02 2024 +0100
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/coyote/http2/LocalStrings.properties | 3 +-
java/org/apache/coyote/http2/Stream.java | 50 ++++++++++++++++++----
java/org/apache/coyote/http2/StreamProcessor.java | 11 ++---
webapps/docs/changelog.xml | 6 +++
4 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties
b/java/org/apache/coyote/http2/LocalStrings.properties
index 48c7ab33f6..e1fd352878 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 5ddb007d9f..0384c513fe 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -110,6 +110,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 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
} 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 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
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 e33c521b5a..cf33d5ba9a 100644
--- a/java/org/apache/coyote/http2/StreamProcessor.java
+++ b/java/org/apache/coyote/http2/StreamProcessor.java
@@ -87,9 +87,9 @@ class StreamProcessor extends AbstractProcessor {
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 @@ class StreamProcessor extends AbstractProcessor {
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 @@ class StreamProcessor extends AbstractProcessor {
state = SocketState.CLOSED;
} finally {
if (state == SocketState.CLOSED) {
+ stream.recycle();
recycle();
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index fd9af8e75e..5e72d13411 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -164,6 +164,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">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
