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
The following commit(s) were added to refs/heads/8.5.x by this push:
new 6a49194 Expand on fix for BZ 65757 - check for specific request thread
6a49194 is described below
commit 6a49194dac7dfa2edb5e3419c8ce4bb03867a580
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Jan 10 13:40:58 2022 +0000
Expand on fix for BZ 65757 - check for specific request thread
Previous check was for any container thread. New check looks for the
specific container thread currently assigned to the request/response.
---
java/org/apache/coyote/AbstractProtocol.java | 4 ----
java/org/apache/coyote/AsyncStateMachine.java | 10 +++++++---
java/org/apache/coyote/ContainerThreadMarker.java | 3 +++
.../http11/upgrade/UpgradeServletInputStream.java | 5 +++--
.../http11/upgrade/UpgradeServletOutputStream.java | 5 +++--
java/org/apache/coyote/http2/StreamProcessor.java | 3 ---
.../tomcat/util/net/ContainerThreadMarker.java | 3 +++
.../catalina/nonblocking/TestNonBlockingAPI.java | 23 ++++++++++++++++++++--
webapps/docs/changelog.xml | 6 ++++++
9 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/java/org/apache/coyote/AbstractProtocol.java
b/java/org/apache/coyote/AbstractProtocol.java
index 43f465a..4e7cfca 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -783,8 +783,6 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler,
return SocketState.CLOSED;
}
- ContainerThreadMarker.set();
-
try {
if (processor == null) {
String negotiatedProtocol =
wrapper.getNegotiatedProtocol();
@@ -997,8 +995,6 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler,
// with "ERROR" level, so it will show up even on
// less-than-verbose logs.
getLog().error(sm.getString("abstractConnectionHandler.error"), e);
- } finally {
- ContainerThreadMarker.clear();
}
// Make sure socket/processor is removed from the list of current
diff --git a/java/org/apache/coyote/AsyncStateMachine.java
b/java/org/apache/coyote/AsyncStateMachine.java
index c45c6b2..182fab1 100644
--- a/java/org/apache/coyote/AsyncStateMachine.java
+++ b/java/org/apache/coyote/AsyncStateMachine.java
@@ -305,7 +305,8 @@ public class AsyncStateMachine {
public synchronized boolean asyncComplete() {
- if (!ContainerThreadMarker.isContainerThread() &&
+ Request request = processor.getRequest();
+ if ((request == null || !request.isRequestThread()) &&
(state == AsyncState.STARTING || state ==
AsyncState.READ_WRITE_OP)) {
updateState(AsyncState.COMPLETE_PENDING);
return false;
@@ -367,7 +368,8 @@ public class AsyncStateMachine {
public synchronized boolean asyncDispatch() {
- if (!ContainerThreadMarker.isContainerThread() &&
+ Request request = processor.getRequest();
+ if ((request == null || !request.isRequestThread()) &&
(state == AsyncState.STARTING || state ==
AsyncState.READ_WRITE_OP)) {
updateState(AsyncState.DISPATCH_PENDING);
return false;
@@ -436,7 +438,9 @@ public class AsyncStateMachine {
} else {
updateState(AsyncState.ERROR);
}
- return !ContainerThreadMarker.isContainerThread();
+
+ Request request = processor.getRequest();
+ return request == null || !request.isRequestThread();
}
diff --git a/java/org/apache/coyote/ContainerThreadMarker.java
b/java/org/apache/coyote/ContainerThreadMarker.java
index 0ba2ef0..2d651b3 100644
--- a/java/org/apache/coyote/ContainerThreadMarker.java
+++ b/java/org/apache/coyote/ContainerThreadMarker.java
@@ -21,7 +21,10 @@ package org.apache.coyote;
* data from an incoming connection. Application created threads are not
* container threads and neither are threads taken from the container thread
* pool to execute AsyncContext.start(Runnable).
+ *
+ * @deprecated Unused. Will be removed in Tomcat 10.1.x
*/
+@Deprecated
public class ContainerThreadMarker {
public static boolean isContainerThread() {
diff --git
a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
index 387581a..e5d4264 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
@@ -21,7 +21,7 @@ import java.io.IOException;
import javax.servlet.ReadListener;
import javax.servlet.ServletInputStream;
-import org.apache.coyote.ContainerThreadMarker;
+import org.apache.coyote.Request;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.ExceptionUtils;
@@ -106,7 +106,8 @@ public class UpgradeServletInputStream extends
ServletInputStream {
this.listener = listener;
// Container is responsible for first call to onDataAvailable().
- if (ContainerThreadMarker.isContainerThread()) {
+ Request request = processor.getRequest();
+ if (request != null && request.isRequestThread()) {
processor.addDispatch(DispatchType.NON_BLOCKING_READ);
} else {
socketWrapper.registerReadInterest();
diff --git
a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
index 84d8fa5..571fe9c 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
@@ -21,7 +21,7 @@ import java.io.IOException;
import javax.servlet.ServletOutputStream;
import javax.servlet.WriteListener;
-import org.apache.coyote.ContainerThreadMarker;
+import org.apache.coyote.Request;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.ExceptionUtils;
@@ -119,7 +119,8 @@ public class UpgradeServletOutputStream extends
ServletOutputStream {
synchronized (registeredLock) {
registered = true;
// Container is responsible for first call to onDataAvailable().
- if (ContainerThreadMarker.isContainerThread()) {
+ Request request = processor.getRequest();
+ if (request != null && request.isRequestThread()) {
processor.addDispatch(DispatchType.NON_BLOCKING_WRITE);
} else {
socketWrapper.registerWriteInterest();
diff --git a/java/org/apache/coyote/http2/StreamProcessor.java
b/java/org/apache/coyote/http2/StreamProcessor.java
index 5f84524..c8731a6 100644
--- a/java/org/apache/coyote/http2/StreamProcessor.java
+++ b/java/org/apache/coyote/http2/StreamProcessor.java
@@ -25,7 +25,6 @@ import javax.servlet.http.HttpServletResponse;
import org.apache.coyote.AbstractProcessor;
import org.apache.coyote.ActionCode;
import org.apache.coyote.Adapter;
-import org.apache.coyote.ContainerThreadMarker;
import org.apache.coyote.ContinueResponseTiming;
import org.apache.coyote.ErrorState;
import org.apache.coyote.Request;
@@ -70,7 +69,6 @@ class StreamProcessor extends AbstractProcessor {
synchronized (this) {
// HTTP/2 equivalent of AbstractConnectionHandler#process()
without the
// socket <-> processor mapping
- ContainerThreadMarker.set();
SocketState state = SocketState.CLOSED;
try {
state = process(socketWrapper, event);
@@ -122,7 +120,6 @@ class StreamProcessor extends AbstractProcessor {
if (state == SocketState.CLOSED) {
recycle();
}
- ContainerThreadMarker.clear();
}
}
} finally {
diff --git a/java/org/apache/tomcat/util/net/ContainerThreadMarker.java
b/java/org/apache/tomcat/util/net/ContainerThreadMarker.java
index 2ab6f68..7149945 100644
--- a/java/org/apache/tomcat/util/net/ContainerThreadMarker.java
+++ b/java/org/apache/tomcat/util/net/ContainerThreadMarker.java
@@ -21,7 +21,10 @@ package org.apache.tomcat.util.net;
* data from an incoming connection. Application created threads are not
* container threads and neither are threads taken from the container thread
* pool to execute AsyncContext.start(Runnable).
+ *
+ * @deprecated Unused. Will be removed in Tomcat 10.1.x
*/
+@Deprecated
public class ContainerThreadMarker {
private static final ThreadLocal<Boolean> marker = new ThreadLocal<>();
diff --git a/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
b/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
index a7740ef..a61a878 100644
--- a/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
+++ b/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java
@@ -20,6 +20,7 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.Serializable;
+import java.lang.reflect.Field;
import java.net.HttpURLConnection;
import java.net.Socket;
import java.net.URL;
@@ -55,16 +56,17 @@ import org.junit.Test;
import org.apache.catalina.Context;
import org.apache.catalina.Wrapper;
+import org.apache.catalina.core.AsyncContextImpl;
import org.apache.catalina.startup.BytesStreamer;
import org.apache.catalina.startup.SimpleHttpClient;
import org.apache.catalina.startup.TesterServlet;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.catalina.valves.TesterAccessLogValve;
+import org.apache.coyote.Request;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.buf.ByteChunk;
-import org.apache.tomcat.util.net.ContainerThreadMarker;
public class TestNonBlockingAPI extends TomcatBaseTest {
@@ -75,6 +77,7 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
private static final byte[] DATA = new byte[WRITE_SIZE];
private static final int WRITE_PAUSE_MS = 500;
+ private static final Field CTX_REQUEST_FIELD;
static {
// Use this sequence for padding to make it easier to spot errors
@@ -91,6 +94,15 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
System.arraycopy(
hex.getBytes(), 0, DATA, i * blockSize + padSize, hexSize);
}
+
+ Field f = null;
+ try {
+ f = AsyncContextImpl.class.getDeclaredField("request");
+ f.setAccessible(true);
+ } catch (NoSuchFieldException | SecurityException e) {
+ Assert.fail();
+ }
+ CTX_REQUEST_FIELD = f;
}
@@ -720,7 +732,14 @@ public class TestNonBlockingAPI extends TomcatBaseTest {
@Override
public void onDataAvailable() throws IOException {
- if (ContainerThreadMarker.isContainerThread()) {
+ Request coyoteRequest;
+ try {
+ coyoteRequest = ((org.apache.catalina.connector.Request)
CTX_REQUEST_FIELD.get(ctx)).getCoyoteRequest();
+ } catch (IllegalArgumentException | IllegalAccessException e) {
+ throw new IllegalStateException(e);
+ }
+
+ if (coyoteRequest.isRequestThread()) {
containerThreadCount.incrementAndGet();
} else {
nonContainerThreadCount.incrementAndGet();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1c77719..535be2f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -194,6 +194,12 @@
container dispatch to another container thread. (remm)
</fix>
<fix>
+ Expand the fix for <bug>65757</bug> so that rather than just checking
if
+ processing is happening on a container thread, the check is now if
+ processing is happening on the container thread currently allocated to
+ this request/response. (markt)
+ </fix>
+ <fix>
Avoid a potential deadlock during the concurrent processing of incoming
HTTP/2 frames for a stream and that stream being reset. (markt)
</fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
