[tomcat] branch master updated (0e324d1 -> 8abf06d)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch master updated (0e324d1 -> 8abf06d)

markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git.


    from 0e324d1  Add end method for possible cleanups
     new e8cad1a  Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63441
     new ab70de3  Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock
     new 8abf06d  Avoid unnecessary logging when host is down

The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 .../apache/catalina/ha/session/DeltaManager.java   | 24 +++++++---
 .../apache/catalina/ha/session/DeltaSession.java   | 54 ++++++++++++++--------
 .../group/interceptors/TcpFailureDetector.java     |  5 +-
 webapps/docs/changelog.xml                         | 27 +++++++++++
 4 files changed, 83 insertions(+), 27 deletions(-)


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[tomcat] 01/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63441

markt
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 e8cad1a635a97f1d9358bb6fffa3f31768c60e13
Author: Mark Thomas <[hidden email]>
AuthorDate: Thu May 16 10:48:17 2019 +0100

    Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63441
   
    Further streamline the processing of session creation messages in the
    DeltaManager to reduce the possibility of a session update message being
    processed before the session has been created.
---
 .../apache/catalina/ha/session/DeltaManager.java   |  5 +++-
 .../apache/catalina/ha/session/DeltaSession.java   | 32 ++++++++++------------
 webapps/docs/changelog.xml                         | 10 +++++++
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/java/org/apache/catalina/ha/session/DeltaManager.java b/java/org/apache/catalina/ha/session/DeltaManager.java
index ee76f55..9f3abef 100644
--- a/java/org/apache/catalina/ha/session/DeltaManager.java
+++ b/java/org/apache/catalina/ha/session/DeltaManager.java
@@ -468,13 +468,16 @@ public class DeltaManager extends ClusterManagerBase{
      */
     @Override
     public Session createEmptySession() {
-        return getNewDeltaSession() ;
+        return new DeltaSession(this);
     }
 
     /**
      * Get new session class to be used in the doLoad() method.
      * @return a new session
+     *
+     * @deprecated Unused. This will be removed in Tomcat 10.
      */
+    @Deprecated
     protected DeltaSession getNewDeltaSession() {
         return new DeltaSession(this);
     }
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java b/java/org/apache/catalina/ha/session/DeltaSession.java
index d61817e..d25f622 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -102,7 +102,9 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
      */
     public DeltaSession(Manager manager) {
         super(manager);
-        this.resetDeltaRequest();
+        boolean recordAllActions = manager instanceof ClusterManagerBase &&
+                ((ClusterManagerBase)manager).isRecordAllActions();
+        deltaRequest = new DeltaRequest(getIdInternal(), recordAllActions);
     }
 
     // ----------------------------------------------------- ReplicatedMapEntry
@@ -292,7 +294,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
 
     public void setMaxInactiveInterval(int interval, boolean addDeltaRequest) {
         super.maxInactiveInterval = interval;
-        if (addDeltaRequest && (deltaRequest != null)) {
+        if (addDeltaRequest) {
             lock();
             try {
                 deltaRequest.setMaxInactiveInterval(interval);
@@ -315,7 +317,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
 
     public void setNew(boolean isNew, boolean addDeltaRequest) {
         super.setNew(isNew);
-        if (addDeltaRequest && (deltaRequest != null)){
+        if (addDeltaRequest){
             lock();
             try {
                 deltaRequest.setNew(isNew);
@@ -343,7 +345,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         lock();
         try {
             super.setPrincipal(principal);
-            if (addDeltaRequest && (deltaRequest != null))
+            if (addDeltaRequest)
                 deltaRequest.setPrincipal(principal);
         } finally {
             unlock();
@@ -365,8 +367,9 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         lock();
         try {
             super.setAuthType(authType);
-            if (addDeltaRequest && (deltaRequest != null))
+            if (addDeltaRequest) {
                 deltaRequest.setAuthType(authType);
+            }
         } finally {
             unlock();
         }
@@ -512,7 +515,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         lock();
         try {
             super.addSessionListener(listener);
-            if (addDeltaRequest && deltaRequest != null && listener instanceof ReplicatedSessionListener) {
+            if (addDeltaRequest && listener instanceof ReplicatedSessionListener) {
                 deltaRequest.addSessionListener(listener);
             }
         } finally {
@@ -529,7 +532,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         lock();
         try {
             super.removeSessionListener(listener);
-            if (addDeltaRequest && deltaRequest != null && listener instanceof ReplicatedSessionListener) {
+            if (addDeltaRequest && listener instanceof ReplicatedSessionListener) {
                 deltaRequest.removeSessionListener(listener);
             }
         } finally {
@@ -594,21 +597,14 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
     public void resetDeltaRequest() {
         lock();
         try {
-            if (deltaRequest == null) {
-                boolean recordAllActions = manager instanceof ClusterManagerBase &&
-                        ((ClusterManagerBase)manager).isRecordAllActions();
-                deltaRequest = new DeltaRequest(getIdInternal(), recordAllActions);
-            } else {
-                deltaRequest.reset();
-                deltaRequest.setSessionId(getIdInternal());
-            }
+            deltaRequest.reset();
+            deltaRequest.setSessionId(getIdInternal());
         } finally{
             unlock();
         }
     }
 
     public DeltaRequest getDeltaRequest() {
-        if (deltaRequest == null) resetDeltaRequest();
         return deltaRequest;
     }
 
@@ -684,7 +680,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         lock();
         try {
             super.setAttribute(name,value, notify);
-            if (addDeltaRequest && deltaRequest != null && !exclude(name, value)) {
+            if (addDeltaRequest && !exclude(name, value)) {
                 deltaRequest.setAttribute(name, value);
             }
         } finally {
@@ -883,7 +879,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
             if (value == null) return;
 
             super.removeAttributeInternal(name,notify);
-            if (addDeltaRequest && deltaRequest != null && !exclude(name, null)) {
+            if (addDeltaRequest && !exclude(name, null)) {
                 deltaRequest.removeAttribute(name);
             }
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index b548b7c..cfd34e2 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -129,6 +129,16 @@
       </fix>
     </changelog>
   </subsection>
+  <subsection name="Cluster">
+    <changelog>
+      <fix>
+        <bug>63441</bug>: Further streamline the processing of session creation
+        messages in the <code>DeltaManager</code> to reduce the possibility of a
+        session update message being processed before the session has been
+        created. (markt)
+      </fix>
+    </changelog>
+  </subsection>
   <subsection name="Other">
     <changelog>
       <update>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

markt
In reply to this post by markt
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 ab70de3278d5e506661faeb5febf71a061b89179
Author: Mark Thomas <[hidden email]>
AuthorDate: Thu May 16 13:36:39 2019 +0100

    Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock
   
    Refactor the DeltaRequest serialization to reduce the window during
    which the DeltaSession is locked and to remove a potential cause of
    deadlocks during serialization.
---
 .../apache/catalina/ha/session/DeltaManager.java   | 19 ++++++++++++++-----
 .../apache/catalina/ha/session/DeltaSession.java   | 22 ++++++++++++++++++++++
 webapps/docs/changelog.xml                         |  6 ++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/java/org/apache/catalina/ha/session/DeltaManager.java b/java/org/apache/catalina/ha/session/DeltaManager.java
index 9f3abef..7f0df95 100644
--- a/java/org/apache/catalina/ha/session/DeltaManager.java
+++ b/java/org/apache/catalina/ha/session/DeltaManager.java
@@ -39,6 +39,7 @@ import org.apache.catalina.tribes.io.ReplicationStream;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.ExceptionUtils;
+import org.apache.tomcat.util.collections.SynchronizedStack;
 import org.apache.tomcat.util.res.StringManager;
 
 /**
@@ -88,6 +89,7 @@ public class DeltaManager extends ClusterManagerBase{
     private boolean receiverQueue = false ;
     private boolean stateTimestampDrop = true ;
     private volatile long stateTransferCreateSendTime;
+    private SynchronizedStack<DeltaRequest> deltaRequestPool = new SynchronizedStack<>();
 
     // -------------------------------------------------------- stats attributes
 
@@ -952,10 +954,10 @@ public class DeltaManager extends ClusterManagerBase{
      *            whether this method has been called during session expiration
      * @return a SessionMessage to be sent,
      */
-    @SuppressWarnings("null") // session can't be null when it is used
     public ClusterMessage requestCompleted(String sessionId, boolean expires) {
         DeltaSession session = null;
         SessionMessage msg = null;
+        DeltaRequest deltaRequest = null;
         try {
             session = (DeltaSession) findSession(sessionId);
             if (session == null) {
@@ -963,8 +965,12 @@ public class DeltaManager extends ClusterManagerBase{
                 // removed the session from the Manager.
                 return null;
             }
-            DeltaRequest deltaRequest = session.getDeltaRequest();
-            session.lock();
+            DeltaRequest newDeltaRequest = deltaRequestPool.pop();
+            if (newDeltaRequest == null) {
+                // Will be configured in replaceDeltaRequest()
+                newDeltaRequest = new DeltaRequest();
+            }
+            deltaRequest = session.replaceDeltaRequest(newDeltaRequest);
             if (deltaRequest.getSize() > 0) {
                 counterSend_EVT_SESSION_DELTA++;
                 byte[] data = serializeDeltaRequest(session,deltaRequest);
@@ -973,14 +979,17 @@ public class DeltaManager extends ClusterManagerBase{
                                              data,
                                              sessionId,
                                              sessionId + "-" + System.currentTimeMillis());
-                session.resetDeltaRequest();
             }
         } catch (IOException x) {
             log.error(sm.getString("deltaManager.createMessage.unableCreateDeltaRequest",
                     sessionId), x);
             return null;
         } finally {
-            if (session!=null) session.unlock();
+            if (deltaRequest != null) {
+                // Reset the instance before it is returned to the pool
+                deltaRequest.reset();
+                deltaRequestPool.push(deltaRequest);
+            }
         }
         if(msg == null) {
             if(!expires && !session.isPrimarySession()) {
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java b/java/org/apache/catalina/ha/session/DeltaSession.java
index d25f622..8a186ce 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -608,6 +608,26 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         return deltaRequest;
     }
 
+    /**
+     * Replace the existing deltaRequest with the provided replacement.
+     *
+     * @param deltaRequest The new deltaRequest. Expected to be either a newly
+     *                     created object or an instance that has been reset.
+     *
+     * @return The old deltaRequest
+     */
+    DeltaRequest replaceDeltaRequest(DeltaRequest deltaRequest) {
+        lock();
+        try {
+            DeltaRequest oldDeltaRequest = this.deltaRequest;
+            this.deltaRequest = deltaRequest;
+            this.deltaRequest.setSessionId(getIdInternal());
+            return oldDeltaRequest;
+        } finally {
+            unlock();
+        }
+    }
+
 
     // ------------------------------------------------- HttpSession Properties
 
@@ -668,6 +688,8 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
 
     public void setAttribute(String name, Object value, boolean notify,boolean addDeltaRequest) {
 
+        log.info("setAttribute name [" + name + ", value [" + value + "]");
+
         // Name cannot be null
         if (name == null) throw new IllegalArgumentException(sm.getString("standardSession.setAttribute.namenull"));
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index cfd34e2..c08fa7e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -132,6 +132,12 @@
   <subsection name="Cluster">
     <changelog>
       <fix>
+        <bug>62841</bug>: Refactor the <code>DeltaRequest</code> serialization
+        to reduce the window during which the <code>DeltaSession</code> is
+        locked and to remove a potential cause of deadlocks during
+        serialization. (markt)
+      </fix>
+      <fix>
         <bug>63441</bug>: Further streamline the processing of session creation
         messages in the <code>DeltaManager</code> to reduce the possibility of a
         session update message being processed before the session has been


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[tomcat] 03/03: Avoid unnecessary logging when host is down

markt
In reply to this post by markt
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 8abf06d9b1af3fd34892966411acf12ae4b7eb00
Author: Mark Thomas <[hidden email]>
AuthorDate: Thu May 16 13:54:25 2019 +0100

    Avoid unnecessary logging when host is down
---
 .../tribes/group/interceptors/TcpFailureDetector.java         |  5 ++---
 webapps/docs/changelog.xml                                    | 11 +++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/java/org/apache/catalina/tribes/group/interceptors/TcpFailureDetector.java b/java/org/apache/catalina/tribes/group/interceptors/TcpFailureDetector.java
index 281ee4d..a0b9b1d 100644
--- a/java/org/apache/catalina/tribes/group/interceptors/TcpFailureDetector.java
+++ b/java/org/apache/catalina/tribes/group/interceptors/TcpFailureDetector.java
@@ -19,6 +19,7 @@ package org.apache.catalina.tribes.group.interceptors;
 import java.net.ConnectException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
+import java.net.NoRouteToHostException;
 import java.net.Socket;
 import java.net.SocketTimeoutException;
 import java.util.Arrays;
@@ -354,9 +355,7 @@ public class TcpFailureDetector extends ChannelInterceptorBase implements TcpFai
                 }
             }//end if
             return true;
-        } catch (SocketTimeoutException sx) {
-            //do nothing, we couldn't connect
-        } catch (ConnectException cx) {
+        } catch (SocketTimeoutException | ConnectException | NoRouteToHostException noop) {
             //do nothing, we couldn't connect
         } catch (Exception x) {
             log.error(sm.getString("tcpFailureDetector.failureDetection.failed", mbr),x);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index c08fa7e..b756017 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -145,6 +145,17 @@
       </fix>
     </changelog>
   </subsection>
+  <subsection name="Tribes">
+    <changelog>
+      <fix>
+        Treat <code>NoRouteToHostException</code> the same way as
+        <code>SocketTimeoutException</code> when checking the health of group
+        members. This avoids a SEVERE log message every time the check is
+        performed when the host associated with a group member is not powered
+        on. (markt)
+      </fix>
+    </changelog>
+  </subsection>
   <subsection name="Other">
     <changelog>
       <update>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

Keiichi Fujino-2
In reply to this post by markt
2019年5月16日(木) 21:55 <[hidden email]>:
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 ab70de3278d5e506661faeb5febf71a061b89179
Author: Mark Thomas <[hidden email]>
AuthorDate: Thu May 16 13:36:39 2019 +0100

    Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

    Refactor the DeltaRequest serialization to reduce the window during
    which the DeltaSession is locked and to remove a potential cause of
    deadlocks during serialization.
---
 .../apache/catalina/ha/session/DeltaManager.java   | 19 ++++++++++++++-----
 .../apache/catalina/ha/session/DeltaSession.java   | 22 ++++++++++++++++++++++
 webapps/docs/changelog.xml                         |  6 ++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/java/org/apache/catalina/ha/session/DeltaManager.java b/java/org/apache/catalina/ha/session/DeltaManager.java
index 9f3abef..7f0df95 100644
--- a/java/org/apache/catalina/ha/session/DeltaManager.java
+++ b/java/org/apache/catalina/ha/session/DeltaManager.java
@@ -39,6 +39,7 @@ import org.apache.catalina.tribes.io.ReplicationStream;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.ExceptionUtils;
+import org.apache.tomcat.util.collections.SynchronizedStack;
 import org.apache.tomcat.util.res.StringManager;

 /**
@@ -88,6 +89,7 @@ public class DeltaManager extends ClusterManagerBase{
     private boolean receiverQueue = false ;
     private boolean stateTimestampDrop = true ;
     private volatile long stateTransferCreateSendTime;
+    private SynchronizedStack<DeltaRequest> deltaRequestPool = new SynchronizedStack<>();

     // -------------------------------------------------------- stats attributes

@@ -952,10 +954,10 @@ public class DeltaManager extends ClusterManagerBase{
      *            whether this method has been called during session expiration
      * @return a SessionMessage to be sent,
      */
-    @SuppressWarnings("null") // session can't be null when it is used
     public ClusterMessage requestCompleted(String sessionId, boolean expires) {
         DeltaSession session = null;
         SessionMessage msg = null;
+        DeltaRequest deltaRequest = null;
         try {
             session = (DeltaSession) findSession(sessionId);
             if (session == null) {
@@ -963,8 +965,12 @@ public class DeltaManager extends ClusterManagerBase{
                 // removed the session from the Manager.
                 return null;
             }
-            DeltaRequest deltaRequest = session.getDeltaRequest();
-            session.lock();
+            DeltaRequest newDeltaRequest = deltaRequestPool.pop();
+            if (newDeltaRequest == null) {
+                // Will be configured in replaceDeltaRequest()
+                newDeltaRequest = new DeltaRequest();
+            }
+            deltaRequest = session.replaceDeltaRequest(newDeltaRequest);
             if (deltaRequest.getSize() > 0) {
                 counterSend_EVT_SESSION_DELTA++;
                 byte[] data = serializeDeltaRequest(session,deltaRequest);
@@ -973,14 +979,17 @@ public class DeltaManager extends ClusterManagerBase{
                                              data,
                                              sessionId,
                                              sessionId + "-" + System.currentTimeMillis());
-                session.resetDeltaRequest();
             }
         } catch (IOException x) {
             log.error(sm.getString("deltaManager.createMessage.unableCreateDeltaRequest",
                     sessionId), x);
             return null;
         } finally {
-            if (session!=null) session.unlock();
+            if (deltaRequest != null) {
+                // Reset the instance before it is returned to the pool
+                deltaRequest.reset();
+                deltaRequestPool.push(deltaRequest);
+            }
         }
         if(msg == null) {
             if(!expires && !session.isPrimarySession()) {
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java b/java/org/apache/catalina/ha/session/DeltaSession.java
index d25f622..8a186ce 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -608,6 +608,26 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         return deltaRequest;
     }

+    /**
+     * Replace the existing deltaRequest with the provided replacement.
+     *
+     * @param deltaRequest The new deltaRequest. Expected to be either a newly
+     *                     created object or an instance that has been reset.
+     *
+     * @return The old deltaRequest
+     */
+    DeltaRequest replaceDeltaRequest(DeltaRequest deltaRequest) {
+        lock();
+        try {
+            DeltaRequest oldDeltaRequest = this.deltaRequest;
+            this.deltaRequest = deltaRequest;
+            this.deltaRequest.setSessionId(getIdInternal());
+            return oldDeltaRequest;
+        } finally {
+            unlock();
+        }
+    }
+


In DeltaManager#serializeDeltaRequest, the session lock has been is obtained.
Do we need to remove it? or replace to deltaRequest.serialize();

Do you have any plan for applying this for using BackupManager ?
There are similar codes in AbstractReplicatedMap#replicate.
Do we need to apply a similar fix to AbstractReplicatedMap#replicate ?


 
     // ------------------------------------------------- HttpSession Properties

@@ -668,6 +688,8 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus

     public void setAttribute(String name, Object value, boolean notify,boolean addDeltaRequest) {

+        log.info("setAttribute name [" + name + ", value [" + value + "]");
+
         // Name cannot be null
         if (name == null) throw new IllegalArgumentException(sm.getString("standardSession.setAttribute.namenull"));

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index cfd34e2..c08fa7e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -132,6 +132,12 @@
   <subsection name="Cluster">
     <changelog>
       <fix>
+        <bug>62841</bug>: Refactor the <code>DeltaRequest</code> serialization
+        to reduce the window during which the <code>DeltaSession</code> is
+        locked and to remove a potential cause of deadlocks during
+        serialization. (markt)
+      </fix>
+      <fix>
         <bug>63441</bug>: Further streamline the processing of session creation
         messages in the <code>DeltaManager</code> to reduce the possibility of a
         session update message being processed before the session has been


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



--
Keiichi.Fujino
Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

Mark Thomas
On 21/05/2019 09:03, Keiichi Fujino wrote:

> 2019年5月16日(木) 21:55 <[hidden email] <mailto:[hidden email]>>:
>
>     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 ab70de3278d5e506661faeb5febf71a061b89179
>     Author: Mark Thomas <[hidden email] <mailto:[hidden email]>>
>     AuthorDate: Thu May 16 13:36:39 2019 +0100
>
>         Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss
>     deadlock
>
>         Refactor the DeltaRequest serialization to reduce the window during
>         which the DeltaSession is locked and to remove a potential cause of
>         deadlocks during serialization.

<snip/>

> In DeltaManager#serializeDeltaRequest, the session lock has been is
> obtained.
> Do we need to remove it? or replace to deltaRequest.serialize();

You are correct. The deadlock is still going to occur. I think we need
to deprecate serializeDeltaRequest().

> Do you have any plan for applying this for using BackupManager ?
> There are similar codes in AbstractReplicatedMap#replicate.
> Do we need to apply a similar fix to AbstractReplicatedMap#replicate ?

Probably. I'll trace the call hierarchy back from
DeltaRequest.serialize() and ensure nothing is holding the session lock.

Thanks for the review.

Mark

>
>
>  
>
>          // -------------------------------------------------
>     HttpSession Properties
>
>     @@ -668,6 +688,8 @@ public class DeltaSession extends
>     StandardSession implements Externalizable,Clus
>
>          public void setAttribute(String name, Object value, boolean
>     notify,boolean addDeltaRequest) {
>
>     +        log.info <http://log.info>("setAttribute name [" + name +
>     ", value [" + value + "]");
>     +
>              // Name cannot be null
>              if (name == null) throw new
>     IllegalArgumentException(sm.getString("standardSession.setAttribute.namenull"));
>
>     diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
>     index cfd34e2..c08fa7e 100644
>     --- a/webapps/docs/changelog.xml
>     +++ b/webapps/docs/changelog.xml
>     @@ -132,6 +132,12 @@
>        <subsection name="Cluster">
>          <changelog>
>            <fix>
>     +        <bug>62841</bug>: Refactor the <code>DeltaRequest</code>
>     serialization
>     +        to reduce the window during which the
>     <code>DeltaSession</code> is
>     +        locked and to remove a potential cause of deadlocks during
>     +        serialization. (markt)
>     +      </fix>
>     +      <fix>
>              <bug>63441</bug>: Further streamline the processing of
>     session creation
>              messages in the <code>DeltaManager</code> to reduce the
>     possibility of a
>              session update message being processed before the session
>     has been
>
>
>     ---------------------------------------------------------------------
>     To unsubscribe, e-mail: [hidden email]
>     <mailto:[hidden email]>
>     For additional commands, e-mail: [hidden email]
>     <mailto:[hidden email]>
>
>
>
> --
> Keiichi.Fujino


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

markt
On 21/05/2019 13:34, Mark Thomas wrote:

> On 21/05/2019 09:03, Keiichi Fujino wrote:
>> 2019年5月16日(木) 21:55 <[hidden email] <mailto:[hidden email]>>:
>>
>>     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 ab70de3278d5e506661faeb5febf71a061b89179
>>     Author: Mark Thomas <[hidden email] <mailto:[hidden email]>>
>>     AuthorDate: Thu May 16 13:36:39 2019 +0100
>>
>>         Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss
>>     deadlock
>>
>>         Refactor the DeltaRequest serialization to reduce the window during
>>         which the DeltaSession is locked and to remove a potential cause of
>>         deadlocks during serialization.

<snip/>
>> Do you have any plan for applying this for using BackupManager ?
>> There are similar codes in AbstractReplicatedMap#replicate.
>> Do we need to apply a similar fix to AbstractReplicatedMap#replicate ?
>
> Probably. I'll trace the call hierarchy back from
> DeltaRequest.serialize() and ensure nothing is holding the session lock.

This is proving much trickier than it first appeared.

I've had to do a fair bit of refactoring to fix this for the
DeltaManager. My first (untested) attempt is here:

https://github.com/markt-asf/tomcat/commits/bz62481-backup-manager
(the final 3 commits)

It is a bit too invasive for me to be comfortable just committing it. I
also want to run the unit tests (although they don't test this code much).

Feedback welcome.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

Keiichi Fujino-2
2019年5月22日(水) 6:43 Mark Thomas <[hidden email]>:
On 21/05/2019 13:34, Mark Thomas wrote:
> On 21/05/2019 09:03, Keiichi Fujino wrote:
>> 2019年5月16日(木) 21:55 <[hidden email] <mailto:[hidden email]>>:
>>
>>     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 ab70de3278d5e506661faeb5febf71a061b89179
>>     Author: Mark Thomas <[hidden email] <mailto:[hidden email]>>
>>     AuthorDate: Thu May 16 13:36:39 2019 +0100
>>
>>         Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss
>>     deadlock
>>
>>         Refactor the DeltaRequest serialization to reduce the window during
>>         which the DeltaSession is locked and to remove a potential cause of
>>         deadlocks during serialization.

<snip/>
>> Do you have any plan for applying this for using BackupManager ?
>> There are similar codes in AbstractReplicatedMap#replicate.
>> Do we need to apply a similar fix to AbstractReplicatedMap#replicate ?
>
> Probably. I'll trace the call hierarchy back from
> DeltaRequest.serialize() and ensure nothing is holding the session lock.

This is proving much trickier than it first appeared.

I've had to do a fair bit of refactoring to fix this for the
DeltaManager. My first (untested) attempt is here:

https://github.com/markt-asf/tomcat/commits/bz62481-backup-manager
(the final 3 commits)

It is a bit too invasive for me to be comfortable just committing it. I
also want to run the unit tests (although they don't test this code much).

Feedback welcome.


It seems that the recordAllActions flag is not set in the newly created DeltaRequest.

There are duplicated codes in DeltaManager#requestCompleted and DeltaSession#getDiff.
It may be able to call getDiff method in the DeltaManager#requestCompleted.
The same is true for DeltaManager#deserializeAndExecuteDeltaRequest and applyDiff.

 
Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



--
Keiichi.Fujino
Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

markt
On 22/05/2019 07:37, Keiichi Fujino wrote:

<snip/>

> It seems that the recordAllActions flag is not set in the newly created
> DeltaRequest.

I reworked the patch multiple times and forgot that for this iteration.
Thanks for catching it. I've fixed this with an additional commit.

> There are duplicated codes in DeltaManager#requestCompleted and
> DeltaSession#getDiff.
> It may be able to call getDiff method in the DeltaManager#requestCompleted.

Good call. Fixed.

> The same is true for DeltaManager#deserializeAndExecuteDeltaRequest and
> applyDiff.

I couldn't see this. There are some similarities but don't see how this
could work.

The unit tests passed so I plan to commit (and back-port) this unless
there are objections.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

Keiichi Fujino-2
2019年5月22日(水) 20:27 Mark Thomas <[hidden email]>:
On 22/05/2019 07:37, Keiichi Fujino wrote:

<snip/>

> It seems that the recordAllActions flag is not set in the newly created
> DeltaRequest.

I reworked the patch multiple times and forgot that for this iteration.
Thanks for catching it. I've fixed this with an additional commit.

> There are duplicated codes in DeltaManager#requestCompleted and
> DeltaSession#getDiff.
> It may be able to call getDiff method in the DeltaManager#requestCompleted.

Good call. Fixed.

> The same is true for DeltaManager#deserializeAndExecuteDeltaRequest and
> applyDiff.

I couldn't see this. There are some similarities but don't see how this
could work.

The unit tests passed so I plan to commit (and back-port) this unless
there are objections.


I have no objection.
Thanks.

 
Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



--
Keiichi.Fujino
Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] 02/03: Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=62841 poss deadlock

markt
On 23/05/2019 03:14, Keiichi Fujino wrote:

> 2019年5月22日(水) 20:27 Mark Thomas <[hidden email]
> <mailto:[hidden email]>>:
>
>     On 22/05/2019 07:37, Keiichi Fujino wrote:
>
>     <snip/>
>
>     > It seems that the recordAllActions flag is not set in the newly
>     created
>     > DeltaRequest.
>
>     I reworked the patch multiple times and forgot that for this iteration.
>     Thanks for catching it. I've fixed this with an additional commit.
>
>     > There are duplicated codes in DeltaManager#requestCompleted and
>     > DeltaSession#getDiff.
>     > It may be able to call getDiff method in the
>     DeltaManager#requestCompleted.
>
>     Good call. Fixed.
>
>     > The same is true for
>     DeltaManager#deserializeAndExecuteDeltaRequest and
>     > applyDiff.
>
>     I couldn't see this. There are some similarities but don't see how this
>     could work.
>
>     The unit tests passed so I plan to commit (and back-port) this unless
>     there are objections.
>
>
> I have no objection.
> Thanks.

Thanks for the review. Much appreciated.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]