2019年5月16日(木) 21:55 <[email protected]>:
> 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 <[email protected]>
> 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: [email protected]
> For additional commands, e-mail: [email protected]
>
>
--
Keiichi.Fujino
