This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 61b2d5d15d46f3f0fead4294108282a1c44a74db
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 e9100e1..84190ac 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -179,6 +179,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()
@@ -207,6 +211,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 d3afe7c..9b8ebb7 100644
--- a/java/org/apache/catalina/connector/InputBuffer.java
+++ b/java/org/apache/catalina/connector/InputBuffer.java
@@ -335,6 +335,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);
@@ -343,9 +344,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;
@@ -355,9 +354,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;
@@ -380,9 +377,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;
@@ -436,10 +431,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;
@@ -450,21 +442,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;
@@ -477,9 +462,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();
@@ -505,9 +488,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;
}
@@ -524,9 +505,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);
@@ -544,15 +523,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);
}
@@ -562,6 +541,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 ac47555..0ea4dbd 100644
--- a/java/org/apache/catalina/connector/OutputBuffer.java
+++ b/java/org/apache/catalina/connector/OutputBuffer.java
@@ -349,6 +349,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 199cc80..219eb5f 100644
--- a/java/org/apache/coyote/Request.java
+++ b/java/org/apache/coyote/Request.java
@@ -162,6 +162,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() {
@@ -566,6 +571,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 d5f95ea..114587f 100644
--- a/java/org/apache/coyote/Response.java
+++ b/java/org/apache/coyote/Response.java
@@ -126,9 +126,9 @@ public final class Response {
private long commitTimeNanos = -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
@@ -285,7 +285,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
*/
@@ -295,7 +296,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
*/
@@ -305,7 +306,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 7f4f930..0ca028b 100644
--- a/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
+++ b/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
@@ -1314,6 +1314,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
@@ -1322,8 +1334,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);
@@ -1334,7 +1345,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");
@@ -1356,7 +1368,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));
}
@@ -1367,12 +1379,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
@@ -1384,7 +1398,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);
}
}
@@ -1425,37 +1440,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 fd5b4e6..364432d 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -146,6 +146,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
