2010/3/24 sebb <seb...@gmail.com>:
> 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
>>
>> @@ -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.
>
That is what "double-checked locking " is about: reading something
twice, once before the lock and once the lock is acquired. I do not
see a need to explicitly document that.
>> @@ -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.
>
That is similar to my idea in the original patch
(2010-03-16_tc6_bug48790.patch),
when I skipped adding sync to this method. My though was that
setMaxActive/DeltaManager and if( size > maxActive ) { maxActive =
size; } block both assign effectively the same value -- the current
count of sessions, so I see no conflict between them.
I will go with Mark's patch, though, because I expect explicit calls
to setMaxActive() to be rare, and so whether there is a sync is not a
big matter.
> 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.
>
A synchronized getter is what we surely do not need here. We can go
back to the separate read and write locks though.
We need volatile so that the current value of 'maxActive' were
consistent between threads. Even the write lock may look a bit
excessive. But - why not?
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
