From 47307ee27abcdea2ee40e33897aca760083de46a Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Tue, 1 Oct 2024 18:27:02 +0100 Subject: [PATCH] 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. --- .../coyote/http2/LocalStrings.properties | 3 +- java/org/apache/coyote/http2/Stream.java | 50 +++++++++++++++---- .../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 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 5ddb007d9f42..0384c513fe71 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 @@ 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 e33c521b5a57..cf33d5ba9a2a 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 fd9af8e75e59..5e72d13411e3 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) + + 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) +