This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 7.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/7.0.x by this push:
new 91aa7e8 Restore the necessary syncs for correct async processing,
91aa7e8 is described below
commit 91aa7e888c64e6aa4c5bf1625024774dbe8f843e
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Oct 16 12:03:44 2019 +0100
Restore the necessary syncs for correct async processing,
Disable the tests that deadlock due to the limitations of the 7.0.x
implementation. These limitations are unlikely to occur in real
applications. They require latches to attempt to force a specific
processing order.
Add code comments for the benefit of future maintainers.
---
java/org/apache/tomcat/util/net/AprEndpoint.java | 59 ++++++++++++----------
java/org/apache/tomcat/util/net/JIoEndpoint.java | 55 +++++++++++---------
.../core/TestAsyncContextStateChanges.java | 9 ++++
3 files changed, 71 insertions(+), 52 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java
b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 10f1eb9..24014d4 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -975,33 +975,38 @@ public class AprEndpoint extends AbstractEndpoint<Long> {
public void processSocketAsync(SocketWrapper<Long> socket,
SocketStatus status) {
try {
- if (waitingRequests.remove(socket)) {
- SocketProcessor proc = new SocketProcessor(socket, status);
- ClassLoader loader =
Thread.currentThread().getContextClassLoader();
- try {
- //threads should not be created by the webapp classloader
- if (Constants.IS_SECURITY_ENABLED) {
- PrivilegedAction<Void> pa = new PrivilegedSetTccl(
- getClass().getClassLoader());
- AccessController.doPrivileged(pa);
- } else {
- Thread.currentThread().setContextClassLoader(
- getClass().getClassLoader());
- }
- Executor executor = getExecutor();
- if (executor == null) {
- log.warn(sm.getString("endpoint.warn.noExector",
- socket, status));
- return;
- } else {
- executor.execute(proc);
- }
- } finally {
- if (Constants.IS_SECURITY_ENABLED) {
- PrivilegedAction<Void> pa = new
PrivilegedSetTccl(loader);
- AccessController.doPrivileged(pa);
- } else {
- Thread.currentThread().setContextClassLoader(loader);
+ // Sync is necessary to ensure that the original processing thread
+ // has placed the socket in waitingRequests before the dispatching
+ // thread tries to use it.
+ synchronized (socket) {
+ if (waitingRequests.remove(socket)) {
+ SocketProcessor proc = new SocketProcessor(socket, status);
+ ClassLoader loader =
Thread.currentThread().getContextClassLoader();
+ try {
+ //threads should not be created by the webapp
classloader
+ if (Constants.IS_SECURITY_ENABLED) {
+ PrivilegedAction<Void> pa = new PrivilegedSetTccl(
+ getClass().getClassLoader());
+ AccessController.doPrivileged(pa);
+ } else {
+ Thread.currentThread().setContextClassLoader(
+ getClass().getClassLoader());
+ }
+ Executor executor = getExecutor();
+ if (executor == null) {
+ log.warn(sm.getString("endpoint.warn.noExector",
+ socket, status));
+ return;
+ } else {
+ executor.execute(proc);
+ }
+ } finally {
+ if (Constants.IS_SECURITY_ENABLED) {
+ PrivilegedAction<Void> pa = new
PrivilegedSetTccl(loader);
+ AccessController.doPrivileged(pa);
+ } else {
+
Thread.currentThread().setContextClassLoader(loader);
+ }
}
}
}
diff --git a/java/org/apache/tomcat/util/net/JIoEndpoint.java
b/java/org/apache/tomcat/util/net/JIoEndpoint.java
index 024de03..b4abeaa 100644
--- a/java/org/apache/tomcat/util/net/JIoEndpoint.java
+++ b/java/org/apache/tomcat/util/net/JIoEndpoint.java
@@ -559,31 +559,36 @@ public class JIoEndpoint extends AbstractEndpoint<Socket>
{
public void processSocketAsync(SocketWrapper<Socket> socket,
SocketStatus status) {
try {
- if (waitingRequests.remove(socket)) {
- SocketProcessor proc = new SocketProcessor(socket,status);
- ClassLoader loader =
Thread.currentThread().getContextClassLoader();
- try {
- //threads should not be created by the webapp classloader
- if (Constants.IS_SECURITY_ENABLED) {
- PrivilegedAction<Void> pa = new PrivilegedSetTccl(
- getClass().getClassLoader());
- AccessController.doPrivileged(pa);
- } else {
- Thread.currentThread().setContextClassLoader(
- getClass().getClassLoader());
- }
- // During shutdown, executor may be null - avoid NPE
- if (!running) {
- return;
- }
- getExecutor().execute(proc);
- //TODO gotta catch RejectedExecutionException and properly
handle it
- } finally {
- if (Constants.IS_SECURITY_ENABLED) {
- PrivilegedAction<Void> pa = new
PrivilegedSetTccl(loader);
- AccessController.doPrivileged(pa);
- } else {
- Thread.currentThread().setContextClassLoader(loader);
+ // Sync is necessary to ensure that the original processing thread
+ // has placed the socket in waitingRequests before the dispatching
+ // thread tries to use it.
+ synchronized (socket) {
+ if (waitingRequests.remove(socket)) {
+ SocketProcessor proc = new SocketProcessor(socket,status);
+ ClassLoader loader =
Thread.currentThread().getContextClassLoader();
+ try {
+ //threads should not be created by the webapp
classloader
+ if (Constants.IS_SECURITY_ENABLED) {
+ PrivilegedAction<Void> pa = new PrivilegedSetTccl(
+ getClass().getClassLoader());
+ AccessController.doPrivileged(pa);
+ } else {
+ Thread.currentThread().setContextClassLoader(
+ getClass().getClassLoader());
+ }
+ // During shutdown, executor may be null - avoid NPE
+ if (!running) {
+ return;
+ }
+ getExecutor().execute(proc);
+ //TODO gotta catch RejectedExecutionException and
properly handle it
+ } finally {
+ if (Constants.IS_SECURITY_ENABLED) {
+ PrivilegedAction<Void> pa = new
PrivilegedSetTccl(loader);
+ AccessController.doPrivileged(pa);
+ } else {
+
Thread.currentThread().setContextClassLoader(loader);
+ }
}
}
}
diff --git a/test/org/apache/catalina/core/TestAsyncContextStateChanges.java
b/test/org/apache/catalina/core/TestAsyncContextStateChanges.java
index 5c2001f..836af28 100644
--- a/test/org/apache/catalina/core/TestAsyncContextStateChanges.java
+++ b/test/org/apache/catalina/core/TestAsyncContextStateChanges.java
@@ -60,6 +60,15 @@ public class TestAsyncContextStateChanges extends
TomcatBaseTest {
List<Object[]> parameterSets = new ArrayList<Object[]>();
for (AsyncEnd asyncEnd : AsyncEnd.values()) {
for (EndTiming endTiming : EndTiming.values()) {
+ if (endTiming == EndTiming.THREAD_BEFORE_EXIT &&
asyncEnd.isError()) {
+ // Skip these tests for Tomcat 7 as they deadlock due to
+ // the write on the non-container thread being unable to
+ // progress until Servlet.service() exists since both
+ // require a lock on the socket.
+ // Note: Connector refactoring in 8.5.x onwards has removed
+ // this limitation
+ continue;
+ }
parameterSets.add(new Object[] { asyncEnd, endTiming });
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
