This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 146f94f87ea398fb592c7a20a5ccbef95e9dd72b
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 b2a36fc2e2..c8596933be 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 @@ 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 fe5bb2ade0..caa3debea3 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 3724839299..a2b6ee452f 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)
</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
