You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2026/10/13 12:18:14 UTC
(tomcat) branch 10.1.x updated (baba50f52c -> e723ca8006)
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a change to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
from baba50f52c Report end of stream from the buffered input filter
new 0bc3edaaa3 Fix end-of-input and ReadListener call-back contract for upgraded NIO connections
new 685f4f1f22 Report errors to upgraded connection WriteListeners at most once
new 7df733c93f Always de-register upgraded connection statistics when the container closes the connection
new cfe280e221 Fix zero-length read handling in the input stream of an upgraded connection
new 1ec79f40ab API compatibility
new e723ca8006 API compatibility
The 6 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../http11/upgrade/UpgradeProcessorBase.java | 5 +-
.../http11/upgrade/UpgradeProcessorExternal.java | 11 +
.../http11/upgrade/UpgradeServletInputStream.java | 31 +-
.../http11/upgrade/UpgradeServletOutputStream.java | 14 +-
java/org/apache/tomcat/util/net/NioEndpoint.java | 13 +-
.../apache/tomcat/util/net/SocketWrapperBase.java | 10 +-
.../apache/coyote/http11/upgrade/TestUpgrade.java | 462 +++++++++++++++++++++
webapps/docs/changelog.xml | 16 +
8 files changed, 548 insertions(+), 14 deletions(-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 01/06: Fix end-of-input and ReadListener call-back contract for upgraded NIO connections
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0bc3edaaa3c7825a1ec39232d5cf45957ce35cb8
Author: opencode <re...@apache.org>
AuthorDate: Tue Oct 13 13:17:37 2026 +0200
Fix end-of-input and ReadListener call-back contract for upgraded NIO
connections
Reporting a clean client EOF (peer FIN) by throwing an EOFException from
NioSocketWrapper.fillReadBuffer() meant that upgraded connections could
not
implement the expected input stream contract: a blocking read threw
instead
of returning -1 and, for non-blocking reads, the clean end of the input
was
reported to the ReadListener as onError(EOFException) rather than as the
normal completion call-back onAllDataRead(). It also made
isReadyForRead()
throw at the end of the input.
NioSocketWrapper now returns -1 from fillReadBuffer() at the end of the
input, which read() propagates to the caller, and isReadyForRead()
reports
the socket as ready in that case since a subsequent read can immediately
report the end of the input. All callers of these methods (HTTP/1.1
input
buffer, AJP, HTTP/2, WebSocket and the upgrade streams) already handle a
-1
return, so the change restores the contract those consumers were written
against. The socket wrapper and processor read javadoc was updated to
document the end-of-input return value.
In UpgradeServletInputStream, an error reported to the ReadListener is
now
terminal: processing stops immediately when checking read readiness
fails
and the new errorReported flag suppresses any further call-backs on
later
dispatches, fixing the previously possible onAllDataRead() after
onError()
ordering. The onAllDataRead() call-back is now also reported at most
once
via the new allDataReadReported flag.
Tests cover the blocking end-of-input return, the non-blocking clean
onAllDataRead() path and the terminal nature of the error and
all-data-read
call-backs.
---
.../http11/upgrade/UpgradeServletInputStream.java | 24 +-
java/org/apache/tomcat/util/net/NioEndpoint.java | 13 +-
.../apache/tomcat/util/net/SocketWrapperBase.java | 10 +-
.../apache/coyote/http11/upgrade/TestUpgrade.java | 303 +++++++++++++++++++++
webapps/docs/changelog.xml | 10 +
5 files changed, 350 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
index ac89fa3a06..1e66986b5b 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
@@ -68,6 +68,18 @@ public class UpgradeServletInputStream extends ServletInputStream {
*/
private volatile Boolean ready = Boolean.TRUE;
+ /**
+ * Whether the error call-back has been triggered. No further call-backs
+ * are permitted once the error call-back has been triggered.
+ */
+ private volatile boolean errorReported = false;
+
+ /**
+ * Whether the all data read call-back has been triggered. It is only
+ * permitted to trigger the all data read call-back once.
+ */
+ private volatile boolean allDataReadReported = false;
+
/**
* The async read listener.
*/
@@ -245,12 +257,20 @@ public class UpgradeServletInputStream extends ServletInputStream {
final void onDataAvailable() {
+ if (errorReported || allDataReadReported) {
+ // No further call-backs are permitted once an error has been
+ // reported or once the end of the data has been reported.
+ return;
+ }
try {
if (listener == null || !socketWrapper.isReadyForRead()) {
return;
}
} catch (IOException ioe) {
onError(ioe);
+ // Once an error has been reported to the listener, no further
+ // listener call-backs are permitted so stop processing here.
+ return;
}
ready = Boolean.TRUE;
ClassLoader oldCL = processor.getUpgradeToken().getContextBind().bind(false, null);
@@ -258,7 +278,8 @@ public class UpgradeServletInputStream extends ServletInputStream {
if (!eof) {
listener.onDataAvailable();
}
- if (eof) {
+ if (eof && !errorReported) {
+ allDataReadReported = true;
listener.onAllDataRead();
}
} catch (Throwable t) {
@@ -274,6 +295,7 @@ public class UpgradeServletInputStream extends ServletInputStream {
if (listener == null) {
return;
}
+ errorReported = true;
ClassLoader oldCL = processor.getUpgradeToken().getContextBind().bind(false, null);
try {
listener.onError(t);
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index a623256639..b5b876b317 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -1561,9 +1561,13 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
return true;
}
- fillReadBuffer(false);
+ int nRead = fillReadBuffer(false);
- return socketBufferHandler.getReadBuffer().position() > 0;
+ // A return of -1 signals the end of the input. Report that the
+ // socket is ready in that case since a subsequent read is able to
+ // report the end of the data to the reader immediately rather than
+ // waiting for data that will never arrive.
+ return nRead == -1 || socketBufferHandler.getReadBuffer().position() > 0;
}
@@ -1700,7 +1704,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
synchronized (readLock) {
n = getSocket().read(buffer);
if (n == -1) {
- throw new EOFException();
+ return -1;
} else if (n == 0) {
// Ensure a spurious wake-up doesn't trigger a duplicate registration
if (!readBlocking) {
@@ -1725,9 +1729,6 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
} while (n == 0); // TLS needs to loop as reading zero application bytes is possible
} else {
n = getSocket().read(buffer);
- if (n == -1) {
- throw new EOFException();
- }
}
return n;
}
diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
index 023193fde4..a8f64ad832 100644
--- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -556,7 +556,8 @@ public abstract class SocketWrapperBase<E> {
* @param off The offset in the array
* @param len The number of bytes to read
*
- * @return the number of bytes read
+ * @return the number of bytes read, or -1 if the end of the input has been
+ * reached
*
* @throws IOException If an I/O error occurs
*/
@@ -568,14 +569,17 @@ public abstract class SocketWrapperBase<E> {
* @param block Whether to block
* @param to The ByteBuffer to read into
*
- * @return the number of bytes read
+ * @return the number of bytes read, or -1 if the end of the input has been
+ * reached
*
* @throws IOException If an I/O error occurs
*/
public abstract int read(boolean block, ByteBuffer to) throws IOException;
/**
- * Checks if the socket is ready for reading.
+ * Checks if the socket is ready for reading. Note that this method may
+ * return {@code true} at the end of the input since, in that case, a
+ * subsequent read is able to report the end of the input immediately.
*
* @return {@code true} if data is available to read
*
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 89e6b81b13..055a48c377 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -49,6 +49,15 @@ import static org.apache.catalina.startup.SimpleHttpClient.CRLF;
import org.apache.catalina.Context;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.coyote.UpgradeToken;
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.ContextBind;
+import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
+import org.apache.tomcat.util.net.SSLSupport;
+import org.apache.tomcat.util.net.SocketEvent;
+import org.apache.tomcat.util.net.SocketWrapperBase;
+import org.easymock.EasyMock;
public class TestUpgrade extends TomcatBaseTest {
@@ -103,6 +112,128 @@ public class TestUpgrade extends TomcatBaseTest {
doTestFixedResponse(FixedResponseNonBlocking.class);
}
+ @Test
+ public void testCleanEofWithBlockingRead() throws Exception {
+ UpgradeConnection uc = doUpgrade(BlockingEof.class);
+
+ // Half-close the connection from the client side. The blocking read in
+ // the upgrade handler should then see the end of the input.
+ uc.shutdownOutput();
+
+ int c = uc.getReader().read();
+
+ // End of input should be signalled by read() returning -1 so the
+ // handler reports normal termination.
+ Assert.assertEquals(BlockingEof.EOF_MARKER, c);
+
+ uc.shutdownInput();
+ }
+
+ @Test
+ public void testCleanEofWithNonBlockingRead() throws Exception {
+ UpgradeConnection uc = doUpgrade(NonBlockingEof.class);
+
+ uc.getWriter().write(MESSAGE);
+ uc.getWriter().flush();
+
+ // Half-close the connection from the client side. The read listener in
+ // the upgrade handler should then see the end of the input.
+ uc.shutdownOutput();
+
+ int c = uc.getReader().read();
+
+ // End of input should be reported via onAllDataRead() (normal
+ // termination marker) rather than via onError().
+ Assert.assertEquals(NonBlockingEof.EOF_MARKER, c);
+
+ uc.shutdownInput();
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testNoCallbacksAfterError() throws Exception {
+ SocketWrapperBase<?> socketWrapper = EasyMock.createMock(SocketWrapperBase.class);
+ ReadListener listener = EasyMock.createMock(ReadListener.class);
+
+ ContextBind contextBind = new ContextBind() {
+ @Override
+ public ClassLoader bind(ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(ClassLoader originalClassLoader) {
+ // NO-OP
+ }
+ };
+ UpgradeProcessorBase processor =
+ new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
+
+ IOException error = new IOException("Deliberately triggered for the test");
+
+ socketWrapper.registerReadInterest();
+ // The first call reports an error. No further socket state check or
+ // listener call-back may occur once the error has been reported.
+ EasyMock.expect(socketWrapper.isReadyForRead()).andThrow(error);
+ listener.onError(error);
+ EasyMock.expectLastCall().once();
+
+ EasyMock.replay(socketWrapper, listener);
+
+ UpgradeServletInputStream sis = new UpgradeServletInputStream(processor, socketWrapper,
+ new UpgradeInfo());
+ sis.setReadListener(listener);
+
+ sis.onDataAvailable();
+ sis.onDataAvailable();
+
+ EasyMock.verify(socketWrapper, listener);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testOnAllDataReadReportedOnlyOnce() throws Exception {
+ SocketWrapperBase<?> socketWrapper = EasyMock.createMock(SocketWrapperBase.class);
+
+ ContextBind contextBind = new ContextBind() {
+ @Override
+ public ClassLoader bind(ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(ClassLoader originalClassLoader) {
+ // NO-OP
+ }
+ };
+ UpgradeProcessorBase processor =
+ new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
+
+ // The first dispatch reports the socket as ready and the subsequent
+ // read by the listener triggers the end of the data. The all data read
+ // call-back must only be reported once, even if further dispatches
+ // occur.
+ socketWrapper.registerReadInterest();
+ EasyMock.expect(socketWrapper.isReadyForRead()).andReturn(Boolean.TRUE);
+ EasyMock.expect(socketWrapper.read(EasyMock.eq(false), EasyMock.anyObject(byte[].class),
+ EasyMock.anyInt(), EasyMock.anyInt())).andReturn(-1);
+
+ EasyMock.replay(socketWrapper);
+
+ UpgradeServletInputStream sis = new UpgradeServletInputStream(processor, socketWrapper,
+ new UpgradeInfo());
+ RecordingReadListener listener = new RecordingReadListener(sis);
+ sis.setReadListener(listener);
+
+ sis.onDataAvailable();
+ sis.onDataAvailable();
+
+ EasyMock.verify(socketWrapper);
+ Assert.assertEquals(1, listener.dataAvailableCount);
+ Assert.assertEquals(1, listener.allDataReadCount);
+ Assert.assertNull(listener.error);
+ }
+
private void doTestCheckClosed(
Class<? extends HttpUpgradeHandler> upgradeHandlerClass)
throws Exception {
@@ -497,6 +628,102 @@ public class TestUpgrade extends TomcatBaseTest {
}
+ public static class BlockingEof implements HttpUpgradeHandler {
+
+ public static final char EOF_MARKER = 'O';
+
+ @Override
+ public void init(WebConnection connection) {
+ try (ServletInputStream sis = connection.getInputStream();
+ ServletOutputStream sos = connection.getOutputStream()) {
+ byte[] buffer = new byte[8192];
+ int read;
+ while ((read = sis.read(buffer)) >= 0) {
+ // Discard any data until the end of the input is reached
+ }
+ // A blocking read should report the end of the input by
+ // returning -1 rather than by throwing an exception.
+ if (read == -1) {
+ sos.write(EOF_MARKER);
+ sos.flush();
+ }
+ } catch (IOException ioe) {
+ throw new IllegalStateException(ioe);
+ }
+ }
+
+ @Override
+ public void destroy() {
+ // NO-OP
+ }
+ }
+
+
+ public static class NonBlockingEof implements HttpUpgradeHandler {
+
+ public static final char EOF_MARKER = 'O';
+ public static final char ERROR_MARKER = 'E';
+
+ @Override
+ public void init(WebConnection connection) {
+ ServletInputStream sis;
+ ServletOutputStream sos;
+ try {
+ sis = connection.getInputStream();
+ sos = connection.getOutputStream();
+ } catch (IOException ioe) {
+ throw new IllegalStateException(ioe);
+ }
+ sis.setReadListener(new EofReadListener(sis, sos));
+ }
+
+ @Override
+ public void destroy() {
+ // NO-OP
+ }
+
+ private static class EofReadListener implements ReadListener {
+
+ private final ServletInputStream sis;
+ private final ServletOutputStream sos;
+
+ EofReadListener(ServletInputStream sis, ServletOutputStream sos) {
+ this.sis = sis;
+ this.sos = sos;
+ }
+
+ @Override
+ public void onDataAvailable() throws IOException {
+ byte[] buffer = new byte[8192];
+ while (sis.isReady()) {
+ if (sis.read(buffer) == -1) {
+ break;
+ }
+ }
+ }
+
+ @Override
+ public void onAllDataRead() throws IOException {
+ // Normal end of input. Report that all data was read.
+ sos.write(EOF_MARKER);
+ sos.flush();
+ sis.close();
+ sos.close();
+ }
+
+ @Override
+ public void onError(Throwable throwable) {
+ try {
+ sos.write(ERROR_MARKER);
+ sos.flush();
+ } catch (IOException ioe) {
+ // Ignore
+ }
+ }
+ }
+ }
+
+
private static class NoOpReadListener implements ReadListener {
@Override
@@ -528,4 +755,80 @@ public class TestUpgrade extends TomcatBaseTest {
// NO-OP
}
}
+
+
+ private static class RecordingReadListener implements ReadListener {
+
+ private final ServletInputStream sis;
+ private final byte[] buffer = new byte[8192];
+
+ private int dataAvailableCount = 0;
+ private int allDataReadCount = 0;
+ private Throwable error = null;
+
+ RecordingReadListener(ServletInputStream sis) {
+ this.sis = sis;
+ }
+
+ @Override
+ public void onDataAvailable() throws IOException {
+ dataAvailableCount++;
+ while (sis.isReady() && sis.read(buffer) > 0) {
+ // Discard the data
+ }
+ }
+
+ @Override
+ public void onAllDataRead() {
+ allDataReadCount++;
+ }
+
+ @Override
+ public void onError(Throwable throwable) {
+ error = throwable;
+ }
+ }
+
+
+ private static class TesterUpgradeProcessor extends UpgradeProcessorBase {
+
+ TesterUpgradeProcessor(UpgradeToken upgradeToken) {
+ super(upgradeToken);
+ }
+
+ @Override
+ protected Log getLog() {
+ return LogFactory.getLog(TesterUpgradeProcessor.class);
+ }
+
+ @Override
+ protected SocketState dispatch(SocketEvent status) {
+ return null;
+ }
+
+ @Override
+ public void close() {
+ // NO-OP
+ }
+
+ @Override
+ public ServletInputStream getInputStream() {
+ return null;
+ }
+
+ @Override
+ public ServletOutputStream getOutputStream() {
+ return null;
+ }
+
+ @Override
+ public void pause() {
+ // NO-OP
+ }
+
+ @Override
+ public void setSslSupport(SSLSupport sslSupport) {
+ // NO-OP
+ }
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1f774ba787..7fe78526d1 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -274,6 +274,16 @@
an application loop that polls <code>available()</code> running.
(remm)
</fix>
+ <fix>
+ Report a clean end of input on an upgraded connection as end of
+ stream, with blocking and non-blocking reads returning
+ <code>-1</code> and the non-blocking <code>ReadListener</code>
+ receiving <code>onAllDataRead()</code>, rather than as an error with
+ an <code>EOFException</code>. Also ensure that no further
+ <code>ReadListener</code> call-backs occur once an error has been
+ reported to the listener, or once the end of the data has been
+ reported. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 06/06: API compatibility
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e723ca80069fc3af2577fb1c02dca3a3a7859663
Author: remm <re...@apache.org>
AuthorDate: Tue Oct 13 14:17:49 2026 +0200
API compatibility
---
.../apache/coyote/http11/upgrade/TestUpgrade.java | 30 ----------------------
1 file changed, 30 deletions(-)
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 9e8437c53a..a4123072ed 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -160,16 +160,6 @@ public class TestUpgrade extends TomcatBaseTest {
ReadListener listener = EasyMock.createMock(ReadListener.class);
ContextBind contextBind = new ContextBind() {
- @Override
- public ClassLoader bind(ClassLoader originalClassLoader) {
- return null;
- }
-
- @Override
- public void unbind(ClassLoader originalClassLoader) {
- // NO-OP
- }
-
@Override
public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
return null;
@@ -210,16 +200,6 @@ public class TestUpgrade extends TomcatBaseTest {
WriteListener listener = EasyMock.createMock(WriteListener.class);
ContextBind contextBind = new ContextBind() {
- @Override
- public ClassLoader bind(ClassLoader originalClassLoader) {
- return null;
- }
-
- @Override
- public void unbind(ClassLoader originalClassLoader) {
- // NO-OP
- }
-
@Override
public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
return null;
@@ -263,16 +243,6 @@ public class TestUpgrade extends TomcatBaseTest {
SocketWrapperBase<?> socketWrapper = EasyMock.createMock(SocketWrapperBase.class);
ContextBind contextBind = new ContextBind() {
- @Override
- public ClassLoader bind(ClassLoader originalClassLoader) {
- return null;
- }
-
- @Override
- public void unbind(ClassLoader originalClassLoader) {
- // NO-OP
- }
-
@Override
public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
return null;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 05/06: API compatibility
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 1ec79f40ab7d02e1f0d8f80c7c3654dd743763ec
Author: remm <re...@apache.org>
AuthorDate: Tue Oct 13 14:12:21 2026 +0200
API compatibility
---
.../apache/coyote/http11/upgrade/TestUpgrade.java | 27 ++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 0f88c70af0..9e8437c53a 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -169,6 +169,15 @@ public class TestUpgrade extends TomcatBaseTest {
public void unbind(ClassLoader originalClassLoader) {
// NO-OP
}
+
+ @Override
+ public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ }
};
UpgradeProcessorBase processor =
new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
@@ -210,6 +219,15 @@ public class TestUpgrade extends TomcatBaseTest {
public void unbind(ClassLoader originalClassLoader) {
// NO-OP
}
+
+ @Override
+ public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ }
};
UpgradeProcessorBase processor =
new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
@@ -254,6 +272,15 @@ public class TestUpgrade extends TomcatBaseTest {
public void unbind(ClassLoader originalClassLoader) {
// NO-OP
}
+
+ @Override
+ public ClassLoader bind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(boolean usePrivilegedAction, ClassLoader originalClassLoader) {
+ }
};
UpgradeProcessorBase processor =
new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 04/06: Fix zero-length read handling in the input stream of an upgraded connection
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit cfe280e221757fe443a4e82d854bf2b2bb93b821
Author: opencode <re...@apache.org>
AuthorDate: Tue Oct 13 14:06:45 2026 +0200
Fix zero-length read handling in the input stream of an upgraded connection
read(byte[], int, int) on UpgradeServletInputStream passed a
zero-length request straight through to the socket wrapper. In
blocking mode that meant the call could block until the client sent
data (the read timeout is infinite for upgraded connections) and, at
the end of the input, consume the end-of-input and return -1, even
though the contract requires a zero-length read to read no bytes and
return zero.
Return zero immediately, after the usual read checks, so the input is
not touched, mirroring the existing handling in readLine().
---
.../http11/upgrade/UpgradeServletInputStream.java | 5 +++
.../apache/coyote/http11/upgrade/TestUpgrade.java | 49 ++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
index 1c78fdbe80..93d0913b29 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
@@ -199,6 +199,11 @@ public class UpgradeServletInputStream extends ServletInputStream {
public final int read(byte[] b, int off, int len) throws IOException {
preReadChecks();
+ // The input must not be touched if the caller requested zero bytes.
+ if (len <= 0) {
+ return 0;
+ }
+
try {
int result = socketWrapper.read(listener == null, b, off, len);
if (result == -1) {
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 56672744ed..0f88c70af0 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -311,6 +311,20 @@ public class TestUpgrade extends TomcatBaseTest {
uc.shutdownOutput();
}
+ @Test
+ public void testZeroLengthReadReturnsZero() throws Exception {
+ UpgradeConnection uc = doUpgrade(ZeroLengthReadBlocking.class);
+
+ // Half-close the connection from the client side. The zero-length read
+ // performed by the handler must neither block waiting for the end of
+ // the input nor report the end of the input.
+ uc.shutdownOutput();
+
+ Assert.assertEquals(ZeroLengthReadBlocking.ZERO_READ_MARKER, uc.getReader().read());
+
+ uc.shutdownInput();
+ }
+
@SuppressWarnings("unchecked")
private static Set<UpgradeInfo> getUpgradeInfos(UpgradeGroupInfo groupInfo) throws Exception {
@@ -536,6 +550,41 @@ public class TestUpgrade extends TomcatBaseTest {
}
+ public static class ZeroLengthReadBlocking implements HttpUpgradeHandler {
+
+ public static final char ZERO_READ_MARKER = '0';
+
+ @Override
+ public void init(WebConnection connection) {
+
+ try (ServletInputStream sis = connection.getInputStream();
+ ServletOutputStream sos = connection.getOutputStream()) {
+
+ // A zero-length read is defined to read no bytes and return
+ // zero, independent of the state of the input.
+ int result = sis.read(new byte[8], 0, 0);
+
+ sos.write(result == 0 ? (byte) ZERO_READ_MARKER : (byte) 'E');
+ sos.flush();
+
+ // Drain the input so the connection is closed cleanly once the
+ // client has finished sending.
+ byte[] buffer = new byte[128];
+ while (sis.read(buffer) >= 0) {
+ // Drain
+ }
+ } catch (IOException ioe) {
+ throw new IllegalStateException(ioe);
+ }
+ }
+
+ @Override
+ public void destroy() {
+ // NO-OP
+ }
+ }
+
+
public static class EchoNonBlocking implements HttpUpgradeHandler {
@Override
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 03/06: Always de-register upgraded connection statistics when the container closes the connection
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 7df733c93f13e7ee168d7f7b49db44cbdda6abda
Author: opencode <re...@apache.org>
AuthorDate: Tue Oct 13 13:59:32 2026 +0200
Always de-register upgraded connection statistics when the container closes the connection
For external upgrade handlers, UpgradeInfo was registered with
UpgradeGroupInfo in the upgrade processor constructor and only
de-registered in the processor's close(), which runs only if the
application closes the WebConnection. Connections closed by the
container never triggered the de-registration, so handlers that do
not close the WebConnection themselves leaked a registered UpgradeInfo
per connection, growing the group's connection set without limit and
keeping the statistics permanently counted as active.
Remove the final modifier from UpgradeProcessorBase.recycle() and
de-register the statistics in the recycle() implementation of
UpgradeProcessorExternal, which is reached on every path on which the
container releases an upgrade processor.
---
.../http11/upgrade/UpgradeProcessorBase.java | 5 +-
.../http11/upgrade/UpgradeProcessorExternal.java | 11 ++++
.../apache/coyote/http11/upgrade/TestUpgrade.java | 68 ++++++++++++++++++++++
3 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorBase.java b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorBase.java
index b0ecddbc96..62e0ae1a99 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorBase.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorBase.java
@@ -67,8 +67,9 @@ public abstract class UpgradeProcessorBase extends AbstractProcessorLight implem
@Override
- public final void recycle() {
- // Currently a NO-OP as upgrade processors are not recycled.
+ public void recycle() {
+ // NO-OP. Sub-classes override this to release resources when the
+ // processor is released by the container.
}
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
index 500d7eb2cc..4109e11fa7 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
@@ -84,6 +84,17 @@ public class UpgradeProcessorExternal extends UpgradeProcessorBase {
}
+ @Override
+ public void recycle() {
+ // Ensure the connection statistics are always de-registered when the
+ // processor is released by the container. The close() method below
+ // de-registers the statistics as well but it is only triggered if the
+ // application closes the WebConnection, which an upgrade handler is
+ // not required to do.
+ upgradeInfo.setGroupInfo(null);
+ }
+
+
// --------------------------------------------------- AutoCloseable methods
@Override
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 84e8f16bd7..56672744ed 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -25,9 +25,12 @@ import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.io.Reader;
import java.io.Writer;
+import java.lang.reflect.Field;
import java.net.Socket;
import java.net.SocketException;
import java.nio.charset.StandardCharsets;
+import java.util.Set;
+import java.util.function.BooleanSupplier;
import javax.net.SocketFactory;
@@ -50,6 +53,7 @@ import org.apache.catalina.Context;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.coyote.UpgradeToken;
+import org.apache.coyote.http11.AbstractHttp11Protocol;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.ContextBind;
@@ -279,6 +283,50 @@ public class TestUpgrade extends TomcatBaseTest {
Assert.assertNull(listener.error);
}
+ @Test
+ public void testUpgradeInfoUnregisteredWhenConnectionClosed() throws Exception {
+ // The upgrade handler never closes the WebConnection so all clean-up
+ // has to be performed by the container when the connection is closed.
+ UpgradeConnection uc = doUpgrade(NoClose.class);
+
+ AbstractHttp11Protocol<?> protocol =
+ (AbstractHttp11Protocol<?>) getTomcatInstance().getConnector().getProtocolHandler();
+ UpgradeGroupInfo groupInfo = protocol.getUpgradeGroupInfo("test");
+
+ Set<UpgradeInfo> upgradeInfos = getUpgradeInfos(groupInfo);
+ waitForCondition(() -> upgradeInfos.size() == 1, 5000);
+ Assert.assertEquals(1, upgradeInfos.size());
+
+ // Closing the connector closes the connection. This is the only event
+ // that can trigger the clean-up of the upgrade registration.
+ getTomcatInstance().stop();
+
+ waitForCondition(upgradeInfos::isEmpty, 10000);
+
+ // The registration must not outlive the connection.
+ Assert.assertEquals("The UpgradeInfo remained registered after the connection was closed.", 0,
+ upgradeInfos.size());
+
+ uc.shutdownInput();
+ uc.shutdownOutput();
+ }
+
+
+ @SuppressWarnings("unchecked")
+ private static Set<UpgradeInfo> getUpgradeInfos(UpgradeGroupInfo groupInfo) throws Exception {
+ Field upgradeInfosField = UpgradeGroupInfo.class.getDeclaredField("upgradeInfos");
+ upgradeInfosField.setAccessible(true);
+ return (Set<UpgradeInfo>) upgradeInfosField.get(groupInfo);
+ }
+
+
+ private static void waitForCondition(BooleanSupplier condition, long timeoutMs) throws InterruptedException {
+ long deadline = System.currentTimeMillis() + timeoutMs;
+ while (!condition.getAsBoolean() && System.currentTimeMillis() < deadline) {
+ Thread.sleep(50);
+ }
+ }
+
private void doTestCheckClosed(
Class<? extends HttpUpgradeHandler> upgradeHandlerClass)
throws Exception {
@@ -468,6 +516,26 @@ public class TestUpgrade extends TomcatBaseTest {
}
+ public static class NoClose implements HttpUpgradeHandler {
+ @Override
+ public void init(WebConnection connection) {
+ // Open the streams to make the upgraded connection fully active but
+ // never close the WebConnection.
+ try {
+ connection.getInputStream();
+ connection.getOutputStream();
+ } catch (IOException ioe) {
+ throw new IllegalStateException(ioe);
+ }
+ }
+
+ @Override
+ public void destroy() {
+ // NO-OP
+ }
+ }
+
+
public static class EchoNonBlocking implements HttpUpgradeHandler {
@Override
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 02/06: Report errors to upgraded connection WriteListeners at most once
Posted by re...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 685f4f1f22a35d67303aa8c2e74732ff7fea57e1
Author: opencode <re...@apache.org>
AuthorDate: Tue Oct 13 13:39:52 2026 +0200
Report errors to upgraded connection WriteListeners at most once
A single failure to flush the output of an upgraded connection was
reported
to the registered WriteListener more than once.
UpgradeServletOutputStream
flushInternal() calls onError() for any Throwable raised by the socket
flush
and then re-throws. onWritePossible() catches that re-thrown exception
and
calls onError() a second time. Additionally the close() performed by
onError() itself re-enters flushInternal(), whose failure triggers a
further
onError(). Since onError() had no one-shot guard and never cleared the
listener, listener.onError() could therefore be invoked repeatedly for
one
failure, contrary to the Servlet spec which allows at most one onError()
and
no call-backs afterwards.
UpgradeServletOutputStream now sets a terminal errorReported flag before
notifying the listener and suppresses any further call-backs, including
onWritePossible() from later OPEN_WRITE dispatches, once an error has
been
reported. The equivalent onError() guard on UpgradeServletInputStream
was
extended in the same way for symmetry.
A new mock-based test verifies that a failing flush reports the error to
the
listener exactly once and that a subsequent onWritePossible() dispatch
triggers no further socket processing.
---
.../http11/upgrade/UpgradeServletInputStream.java | 2 +-
.../http11/upgrade/UpgradeServletOutputStream.java | 14 ++++++-
.../apache/coyote/http11/upgrade/TestUpgrade.java | 45 ++++++++++++++++++++++
webapps/docs/changelog.xml | 6 +++
4 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
index 1e66986b5b..1c78fdbe80 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
@@ -292,7 +292,7 @@ public class UpgradeServletInputStream extends ServletInputStream {
private void onError(Throwable t) {
- if (listener == null) {
+ if (listener == null || errorReported) {
return;
}
errorReported = true;
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
index 5dcced0546..5f6085b920 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
@@ -79,6 +79,12 @@ public class UpgradeServletOutputStream extends ServletOutputStream {
*/
private volatile boolean closed = false;
+ /**
+ * Whether an error has been reported to the listener. No further
+ * call-backs are permitted once the error has been reported.
+ */
+ private volatile boolean errorReported = false;
+
// Start in blocking-mode
/**
* The async write listener.
@@ -248,6 +254,11 @@ public class UpgradeServletOutputStream extends ServletOutputStream {
final void onWritePossible() {
+ if (errorReported) {
+ // No further call-backs are permitted once an error has been
+ // reported.
+ return;
+ }
try {
if (flushing) {
flushInternal(false, true);
@@ -293,9 +304,10 @@ public class UpgradeServletOutputStream extends ServletOutputStream {
private void onError(Throwable t) {
- if (listener == null) {
+ if (listener == null || errorReported) {
return;
}
+ errorReported = true;
ClassLoader oldCL = processor.getUpgradeToken().getContextBind().bind(false, null);
try {
listener.onError(t);
diff --git a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
index 055a48c377..84e8f16bd7 100644
--- a/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
+++ b/test/org/apache/coyote/http11/upgrade/TestUpgrade.java
@@ -190,6 +190,51 @@ public class TestUpgrade extends TomcatBaseTest {
EasyMock.verify(socketWrapper, listener);
}
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testWriteErrorReportedOnlyOnce() throws Exception {
+ SocketWrapperBase<?> socketWrapper = EasyMock.createMock(SocketWrapperBase.class);
+ WriteListener listener = EasyMock.createMock(WriteListener.class);
+
+ ContextBind contextBind = new ContextBind() {
+ @Override
+ public ClassLoader bind(ClassLoader originalClassLoader) {
+ return null;
+ }
+
+ @Override
+ public void unbind(ClassLoader originalClassLoader) {
+ // NO-OP
+ }
+ };
+ UpgradeProcessorBase processor =
+ new TesterUpgradeProcessor(new UpgradeToken(null, contextBind, null, "test"));
+
+ IOException error = new IOException("Deliberately triggered for the test");
+
+ socketWrapper.registerWriteInterest();
+ // The flush fails. That failure is seen twice, once by the caller and
+ // once by the close that reports the error. The listener must only be
+ // notified once.
+ EasyMock.expect(socketWrapper.flush(false)).andThrow(error);
+ EasyMock.expect(socketWrapper.flush(false)).andThrow(error);
+ listener.onError(error);
+ EasyMock.expectLastCall().once();
+
+ EasyMock.replay(socketWrapper, listener);
+
+ UpgradeServletOutputStream sos = new UpgradeServletOutputStream(processor, socketWrapper,
+ new UpgradeInfo());
+ sos.setWriteListener(listener);
+
+ sos.onWritePossible();
+ // The call-back for the next dispatch must not trigger any further
+ // socket processing.
+ sos.onWritePossible();
+
+ EasyMock.verify(socketWrapper, listener);
+ }
+
@Test
@SuppressWarnings("unchecked")
public void testOnAllDataReadReportedOnlyOnce() throws Exception {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 7fe78526d1..bdf8cebf8d 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -284,6 +284,12 @@
reported to the listener, or once the end of the data has been
reported. (remm)
</fix>
+ <fix>
+ Report an error to the <code>WriteListener</code> of an upgraded
+ connection at most once and stop triggering further call-backs once
+ an error has been reported. A single failure to flush the output was
+ previously reported to the listener multiple times. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org