You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/10/14 16:58:50 UTC
[tomcat] 04/04: Remove sync that triggers deadlock with BZ 63816
test case
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
commit fe65d4c1aac1759e99befe417afde51dded567b5
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Oct 14 17:51:56 2019 +0100
Remove sync that triggers deadlock with BZ 63816 test case
I haven't been able to re-create the test failure on Linux that
promoted the adding of this sync. It is sufficiently long ago
that other refactoring may have provided an alternative fix.
---
java/org/apache/tomcat/util/net/JIoEndpoint.java | 52 ++++++++++++------------
1 file changed, 25 insertions(+), 27 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/JIoEndpoint.java b/java/org/apache/tomcat/util/net/JIoEndpoint.java
index 166640a..024de03 100644
--- a/java/org/apache/tomcat/util/net/JIoEndpoint.java
+++ b/java/org/apache/tomcat/util/net/JIoEndpoint.java
@@ -559,33 +559,31 @@ public class JIoEndpoint extends AbstractEndpoint<Socket> {
public void processSocketAsync(SocketWrapper<Socket> socket,
SocketStatus status) {
try {
- 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);
- }
+ 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);
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] 04/04: Remove sync that triggers deadlock with BZ 63816
test case
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2019 17:58, markt@apache.org wrote:
> 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
>
> commit fe65d4c1aac1759e99befe417afde51dded567b5
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Mon Oct 14 17:51:56 2019 +0100
>
> Remove sync that triggers deadlock with BZ 63816 test case
Hmm.
The same deadlock exists with APR.
However, the sync appears to be required to prevent bug 49884.
It looks like I need to spend some more time with the async code. I'll
remove the sync for now on the grounds that a deadlock is worse than a
10s delay but my intention is that that is only a temporary fix.
Mark
>
> I haven't been able to re-create the test failure on Linux that
> promoted the adding of this sync. It is sufficiently long ago
> that other refactoring may have provided an alternative fix.
> ---
> java/org/apache/tomcat/util/net/JIoEndpoint.java | 52 ++++++++++++------------
> 1 file changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/java/org/apache/tomcat/util/net/JIoEndpoint.java b/java/org/apache/tomcat/util/net/JIoEndpoint.java
> index 166640a..024de03 100644
> --- a/java/org/apache/tomcat/util/net/JIoEndpoint.java
> +++ b/java/org/apache/tomcat/util/net/JIoEndpoint.java
> @@ -559,33 +559,31 @@ public class JIoEndpoint extends AbstractEndpoint<Socket> {
> public void processSocketAsync(SocketWrapper<Socket> socket,
> SocketStatus status) {
> try {
> - 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);
> - }
> + 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);
> }
> }
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org