On 24/03/2010, ma...@apache.org <ma...@apache.org> wrote:
> Author: markt
> Date: Wed Mar 24 12:38:23 2010
> New Revision: 927037
>
> URL: http://svn.apache.org/viewvc?rev=927037&view=rev
> Log:
> Simpler fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=48790
> based on a patch by kkolinko
> Make maxActive thread safe. Probably unnecessary but technically a bug.
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
>
> Modified: tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java?rev=927037&r1=927036&r2=927037&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> (original)
> +++ tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java Wed
> Mar 24 12:38:23 2010
> @@ -1160,7 +1160,7 @@ public class DeltaManager extends Cluste
> rejectedSessions = 0 ;
> sessionReplaceCounter = 0 ;
> counterNoStateTransfered = 0 ;
> - maxActive = getActiveSessions() ;
> + setMaxActive(getActiveSessions());
> sessionCounter = getActiveSessions() ;
> counterReceive_EVT_ALL_SESSION_DATA = 0;
> counterReceive_EVT_GET_ALL_SESSIONS = 0;
>
> Modified: tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java?rev=927037&r1=927036&r2=927037&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java Wed Mar
> 24 12:38:23 2010
> @@ -183,7 +183,9 @@ public abstract class ManagerBase extend
> // Number of sessions created by this manager
> protected int sessionCounter=0;
>
> - protected int maxActive=0;
> + protected volatile int maxActive=0;
> +
> + private final Object maxActiveUpdateLock = new Object();
>
> // number of duplicated session ids - anything >0 means we have problems
> protected int duplicates=0;
> @@ -765,7 +767,11 @@ public abstract class ManagerBase extend
> sessions.put(session.getIdInternal(), session);
> int size = sessions.size();
> if( size > maxActive ) {
> - maxActive = size;
> + synchronized(maxActiveUpdateLock) {
> + if( size > maxActive ) {
> + maxActive = size;
> + }
> + }
In the code above, maxActive is read twice, and may change in between reads.
It's probably not a problem, but IMO should be documented that the
possibility has been considered.
> }
> }
>
> @@ -1081,7 +1087,9 @@ public abstract class ManagerBase extend
>
>
> public void setMaxActive(int maxActive) {
> - this.maxActive = maxActive;
> + synchronized (maxActiveUpdateLock) {
> + this.maxActive = maxActive;
> + }
> }
Looks a bit odd to use both volatile and synchronized.
There seems to be no point to the synch. in setMaxActive, given that
the field is volatile.
Also, the field is protected, which allows subclasses to bypass the synch.
It would be safer to make the field private, remove the volatile
qualifier, and add a synchronised getter.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
