This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0a11905b67fdff58f4efa77fb099ca9967a709b3
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Feb 24 18:43:22 2021 +0000
Improve error handling if non-blocking IO code swallows IOExceptions
Tomcat previously expected IOExceptions in onWritePossible() and
onDataAvailable() to not be swallowed. These additions allow correct
error handling even if the application code swallows the exception.
---
.../apache/catalina/connector/CoyoteAdapter.java | 8 +++
.../org/apache/catalina/connector/InputBuffer.java | 57 +++++++----------
.../apache/catalina/connector/OutputBuffer.java | 1 +
java/org/apache/coyote/Request.java | 33 ++++++++++
java/org/apache/coyote/Response.java | 11 ++--
.../catalina/nonblocking/TestNonBlockingAPI.java | 73 ++++++++++++++--------
webapps/docs/changelog.xml | 6 ++
7 files changed, 124 insertions(+), 65 deletions(-)
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java
b/java/org/apache/catalina/connector/CoyoteAdapter.java
index d8359ec..325f2cd 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -190,6 +190,10 @@ public class CoyoteAdapter implements Adapter {
readListener != null) {
readListener.onAllDataRead();
}
+ // User code may have swallowed an IOException
+ if (response.getCoyoteResponse().isExceptionPresent())
{
+ throw
response.getCoyoteResponse().getErrorException();
+ }
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
// Need to trigger the call to
AbstractProcessor.setErrorState()
@@ -218,6 +222,10 @@ public class CoyoteAdapter implements Adapter {
if (request.isFinished() &&
req.sendAllDataReadEvent()) {
readListener.onAllDataRead();
}
+ // User code may have swallowed an IOException
+ if (request.getCoyoteRequest().isExceptionPresent()) {
+ throw
request.getCoyoteRequest().getErrorException();
+ }
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
// Need to trigger the call to
AbstractProcessor.setErrorState()
diff --git a/java/org/apache/catalina/connector/InputBuffer.java
b/java/org/apache/catalina/connector/InputBuffer.java
index 2da5fca..8393e09 100644
--- a/java/org/apache/catalina/connector/InputBuffer.java
+++ b/java/org/apache/catalina/connector/InputBuffer.java
@@ -343,6 +343,7 @@ public class InputBuffer extends Reader
try {
return coyoteRequest.doRead(this);
} catch (IOException ioe) {
+ coyoteRequest.setErrorException(ioe);
// An IOException on a read is almost always due to
// the remote client aborting the request.
throw new ClientAbortException(ioe);
@@ -351,9 +352,7 @@ public class InputBuffer extends Reader
public int readByte() throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (checkByteBufferEof()) {
return -1;
@@ -363,9 +362,7 @@ public class InputBuffer extends Reader
public int read(byte[] b, int off, int len) throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (checkByteBufferEof()) {
return -1;
@@ -388,9 +385,7 @@ public class InputBuffer extends Reader
* @throws IOException if an input or output exception has occurred
*/
public int read(ByteBuffer to) throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (checkByteBufferEof()) {
return -1;
@@ -456,10 +451,7 @@ public class InputBuffer extends Reader
@Override
public int read() throws IOException {
-
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (checkCharBufferEof()) {
return -1;
@@ -470,21 +462,14 @@ public class InputBuffer extends Reader
@Override
public int read(char[] cbuf) throws IOException {
-
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
-
+ throwIfClosed();
return read(cbuf, 0, cbuf.length);
}
@Override
public int read(char[] cbuf, int off, int len) throws IOException {
-
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (checkCharBufferEof()) {
return -1;
@@ -497,9 +482,7 @@ public class InputBuffer extends Reader
@Override
public long skip(long n) throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (n < 0) {
throw new IllegalArgumentException();
@@ -525,9 +508,7 @@ public class InputBuffer extends Reader
@Override
public boolean ready() throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (state == INITIAL_STATE) {
state = CHAR_STATE;
}
@@ -544,9 +525,7 @@ public class InputBuffer extends Reader
@Override
public void mark(int readAheadLimit) throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (cb.remaining() <= 0) {
clear(cb);
@@ -564,15 +543,15 @@ public class InputBuffer extends Reader
@Override
public void reset() throws IOException {
- if (closed) {
- throw new IOException(sm.getString("inputBuffer.streamClosed"));
- }
+ throwIfClosed();
if (state == CHAR_STATE) {
if (markPos < 0) {
clear(cb);
markPos = -1;
- throw new IOException();
+ IOException ioe = new IOException();
+ coyoteRequest.setErrorException(ioe);
+ throw ioe;
} else {
cb.position(markPos);
}
@@ -582,6 +561,14 @@ public class InputBuffer extends Reader
}
+ private void throwIfClosed() throws IOException {
+ if (closed) {
+ IOException ioe = new
IOException(sm.getString("inputBuffer.streamClosed"));
+ coyoteRequest.setErrorException(ioe);
+ throw ioe;
+ }
+ }
+
public void checkConverter() throws IOException {
if (conv != null) {
return;
diff --git a/java/org/apache/catalina/connector/OutputBuffer.java
b/java/org/apache/catalina/connector/OutputBuffer.java
index 6e0e951..df5db81 100644
--- a/java/org/apache/catalina/connector/OutputBuffer.java
+++ b/java/org/apache/catalina/connector/OutputBuffer.java
@@ -369,6 +369,7 @@ public class OutputBuffer extends Writer {
// An IOException on a write is almost always due to
// the remote client aborting the request. Wrap this
// so that it can be handled better by the error dispatcher.
+ coyoteResponse.setErrorException(e);
throw new ClientAbortException(e);
}
}
diff --git a/java/org/apache/coyote/Request.java
b/java/org/apache/coyote/Request.java
index e7c943b..35189e8 100644
--- a/java/org/apache/coyote/Request.java
+++ b/java/org/apache/coyote/Request.java
@@ -161,6 +161,11 @@ public final class Request {
private boolean sendfile = true;
+ /**
+ * Holds request body reading error exception.
+ */
+ private Exception errorException = null;
+
volatile ReadListener listener;
public ReadListener getReadListener() {
@@ -595,6 +600,34 @@ public final class Request {
}
+ // -------------------- Error tracking --------------------
+
+ /**
+ * Set the error Exception that occurred during the writing of the response
+ * processing.
+ *
+ * @param ex The exception that occurred
+ */
+ public void setErrorException(Exception ex) {
+ errorException = ex;
+ }
+
+
+ /**
+ * Get the Exception that occurred during the writing of the response.
+ *
+ * @return The exception that occurred
+ */
+ public Exception getErrorException() {
+ return errorException;
+ }
+
+
+ public boolean isExceptionPresent() {
+ return errorException != null;
+ }
+
+
// -------------------- debug --------------------
@Override
diff --git a/java/org/apache/coyote/Response.java
b/java/org/apache/coyote/Response.java
index 536f50f..1f965bb 100644
--- a/java/org/apache/coyote/Response.java
+++ b/java/org/apache/coyote/Response.java
@@ -122,9 +122,9 @@ public final class Response {
private long commitTime = -1;
/**
- * Holds request error exception.
+ * Holds response writing error exception.
*/
- Exception errorException = null;
+ private Exception errorException = null;
/**
* With the introduction of async processing and the possibility of
@@ -272,7 +272,8 @@ public final class Response {
// -----------------Error State --------------------
/**
- * Set the error Exception that occurred during request processing.
+ * Set the error Exception that occurred during the writing of the response
+ * processing.
*
* @param ex The exception that occurred
*/
@@ -282,7 +283,7 @@ public final class Response {
/**
- * Get the Exception that occurred during request processing.
+ * Get the Exception that occurred during the writing of the response.
*
* @return The exception that occurred
*/
@@ -292,7 +293,7 @@ public final class Response {
public boolean isExceptionPresent() {
- return ( errorException != null );
+ return errorException != null;
}
diff --git a/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
b/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
index bf1635d..c612c4a 100644
--- a/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
+++ b/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
@@ -1317,6 +1317,18 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
}
+ @Test
+ public void testNonBlockingWriteError02NoSwallow() throws Exception {
+ doTestNonBlockingWriteError02(false);
+ }
+
+
+ @Test
+ public void testNonBlockingWriteError02Swallow() throws Exception {
+ doTestNonBlockingWriteError02(true);
+ }
+
+
/*
* Tests client disconnect in the following scenario:
* - async with non-blocking IO
@@ -1325,8 +1337,7 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
* - client disconnects
* - server attempts a write
*/
- @Test
- public void testNonBlockingWriteError02() throws Exception {
+ private void doTestNonBlockingWriteError02(boolean swallowIoException)
throws Exception {
CountDownLatch responseCommitLatch = new CountDownLatch(1);
CountDownLatch clientCloseLatch = new CountDownLatch(1);
CountDownLatch asyncCompleteLatch = new CountDownLatch(1);
@@ -1337,7 +1348,8 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
// No file system docBase required
Context ctx = tomcat.addContext("", null);
- NBWriteServlet02 writeServlet = new
NBWriteServlet02(responseCommitLatch, clientCloseLatch, asyncCompleteLatch);
+ NBWriteServlet02 writeServlet =
+ new NBWriteServlet02(responseCommitLatch, clientCloseLatch,
asyncCompleteLatch, swallowIoException);
Wrapper wrapper = Tomcat.addServlet(ctx, "writeServlet", writeServlet);
wrapper.setAsyncSupported(true);
ctx.addServletMappingDecoded("/*", "writeServlet");
@@ -1359,7 +1371,7 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
client.disconnect();
clientCloseLatch.countDown();
- Assert.assertTrue("Failed to complete async processing",
asyncCompleteLatch.await(10, TimeUnit.SECONDS));
+ Assert.assertTrue("Failed to complete async processing",
asyncCompleteLatch.await(60, TimeUnit.SECONDS));
}
@@ -1370,12 +1382,14 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
private final CountDownLatch responseCommitLatch;
private final CountDownLatch clientCloseLatch;
private final CountDownLatch asyncCompleteLatch;
+ private final boolean swallowIoException;
public NBWriteServlet02(CountDownLatch responseCommitLatch,
CountDownLatch clientCloseLatch,
- CountDownLatch asyncCompleteLatch) {
+ CountDownLatch asyncCompleteLatch, boolean swallowIoException)
{
this.responseCommitLatch = responseCommitLatch;
this.clientCloseLatch = clientCloseLatch;
this.asyncCompleteLatch = asyncCompleteLatch;
+ this.swallowIoException = swallowIoException;
}
@Override
@@ -1387,7 +1401,8 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
ac.addListener(new TestAsyncListener02(asyncCompleteLatch));
ac.setTimeout(5000);
- WriteListener writeListener = new TestWriteListener02(ac,
responseCommitLatch, clientCloseLatch);
+ WriteListener writeListener =
+ new TestWriteListener02(ac, responseCommitLatch,
clientCloseLatch, swallowIoException);
resp.getOutputStream().setWriteListener(writeListener);
}
}
@@ -1428,37 +1443,45 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
private final AsyncContext ac;
private final CountDownLatch responseCommitLatch;
private final CountDownLatch clientCloseLatch;
+ private final boolean swallowIoException;
private volatile int stage = 0;
public TestWriteListener02(AsyncContext ac, CountDownLatch
responseCommitLatch,
- CountDownLatch clientCloseLatch) {
+ CountDownLatch clientCloseLatch, boolean swallowIoException) {
this.ac = ac;
this.responseCommitLatch = responseCommitLatch;
this.clientCloseLatch = clientCloseLatch;
+ this.swallowIoException = swallowIoException;
}
@Override
public void onWritePossible() throws IOException {
- ServletOutputStream sos = ac.getResponse().getOutputStream();
- do {
- if (stage == 0) {
- // Commit the response
- ac.getResponse().flushBuffer();
- responseCommitLatch.countDown();
- stage++;
- } else if (stage == 1) {
- // Wait for the client to drop the connection
- try {
- clientCloseLatch.await();
- } catch (InterruptedException e) {
- // Ignore
+ try {
+ ServletOutputStream sos = ac.getResponse().getOutputStream();
+ do {
+ if (stage == 0) {
+ // Commit the response
+ ac.getResponse().flushBuffer();
+ responseCommitLatch.countDown();
+ stage++;
+ } else if (stage == 1) {
+ // Wait for the client to drop the connection
+ try {
+ clientCloseLatch.await();
+ } catch (InterruptedException e) {
+ // Ignore
+ }
+ sos.print("TEST");
+ stage++;
+ } else if (stage == 2) {
+ sos.flush();
}
- sos.print("TEST");
- stage++;
- } else if (stage == 2) {
- sos.flush();
+ } while (sos.isReady());
+ } catch (IOException ioe) {
+ if (!swallowIoException) {
+ throw ioe;
}
- } while (sos.isReady());
+ }
}
@Override
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1859a3d..c000da3 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -115,6 +115,12 @@
when a I/O error occurs during non-blocking I/O. There were some cases
discovered where this was not happening. (markt)
</fix>
+ <add>
+ Make the non-blocking I/O error handling more robust by handling the
+ case where the application code swallows an <code>IOException</code> in
+ <code>WriteListener.onWritePossible()</code> and
+ <code>ReadListener.onDataAvailable()</code>. (markt)
+ </add>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
