On 29/03/2009, Rainer Jung <rainer.j...@kippdata.de> wrote: > On 29.03.2009 19:02, sebb wrote: > > > On 29/03/2009, rj...@apache.org<rj...@apache.org> wrote: > > > > > Author: rjung > > > Date: Sun Mar 29 14:17:16 2009 > > > New Revision: 759694 > > > > > > URL: http://svn.apache.org/viewvc?rev=759694&view=rev > > > Log: > > > Fix locking in FastAsyncSocketSender: > > > > > > Locking in DataSender and the sub class FastAsyncSocketSender > > > uses the same lock, although the reasons for locking are totally > > > independent. > > > > > > - FastAsyncSocketSender only locks for assuring atomicity of internal > > > statistics counter manipulation. > > > > > > - DataSender locks access to sockets and other things. > > > > > > When a node crashes, sharing the same lock leads the request > > > handling threads to block in the replication valve, because > > > the replication sender thread keeps the lock while trying > > > to establish a new connection to the crashed node. > > > > > > This problem stops, as soon as the membership drops the node, > > > but keeps a growing number of connector threads blocked > > > until that happens. > > > > > > Using a separate mutex for the local statistics manipulations will > > > no longer block the request handling threads. > > > > > > > This is very useful info, but it's going to be tedious to find later > > when looking at the code. > > Or it could even be lost forever - does SVN keep track of log message > changes? > > > > May I suggest that it be added to the comments or Javadoc for the > > class, or perhaps be added to package.html? > > > > Thanks for the suggestion. I would say the comment on the new mutex member > should be enough. The verbose log message above aims at making the the > review of the commit/change easier. The shared lock should have never been > in place. >
Not sure I could glean that information from the mutex Javadoc, but I don't know the code well. Your call. I've just noticed that the mutex is not declared final. It would not serve its purpose if it was updated (and I'm not certain, but it might not actually be published properly otherwise), so it's safer if it's final. There seem to be some other thread-safety issues in the class - for example, keepRunning is not volatile (nor is access synchronized) so there is no guarantee that the backend thread will see the change made by the stopRunning() method. It may work - most of the time. > > > > > > Modified: > > > > tomcat/sandbox/tomcat-oacc/trunk/src/share/org/apache/catalina/cluster/tcp/FastAsyncSocketSender.java > > > > > > Modified: > tomcat/sandbox/tomcat-oacc/trunk/src/share/org/apache/catalina/cluster/tcp/FastAsyncSocketSender.java > > > URL: > http://svn.apache.org/viewvc/tomcat/sandbox/tomcat-oacc/trunk/src/share/org/apache/catalina/cluster/tcp/FastAsyncSocketSender.java?rev=759694&r1=759693&r2=759694&view=diff > > > > ============================================================================== > > > --- > tomcat/sandbox/tomcat-oacc/trunk/src/share/org/apache/catalina/cluster/tcp/FastAsyncSocketSender.java > (original) > > > +++ > tomcat/sandbox/tomcat-oacc/trunk/src/share/org/apache/catalina/cluster/tcp/FastAsyncSocketSender.java > Sun Mar 29 14:17:16 2009 > > > @@ -92,6 +92,13 @@ > > > > > > private int threadPriority = Thread.NORM_PRIORITY;; > > > > > > + /** > > > + * Separate mutex for our local state. > > > + * We keep the synchronization independent of the synchronization > > > + * in the super class DataSender. > > > + */ > > > + private Object mutex = new Object(); > > > + > > > // > ------------------------------------------------------------- > Constructor > > > > > > /** > > > @@ -354,7 +361,7 @@ > > > public void sendMessage(ClusterData data) > > > throws java.io.IOException { > > > queue.add(data.getUniqueId(), data); > > > - synchronized (this) { > > > + synchronized (mutex) { > > > inQueueCounter++; > > > if(queueThread != null) > > > > queueThread.incQueuedNrOfBytes(data.getMessage().length); > > > @@ -456,15 +463,15 @@ > > > return queuedNrOfBytes; > > > } > > > > > > - protected synchronized void setQueuedNrOfBytes(long > queuedNrOfBytes) { > > > + protected void setQueuedNrOfBytes(long queuedNrOfBytes) { > > > this.queuedNrOfBytes = queuedNrOfBytes; > > > } > > > > > > - protected synchronized void incQueuedNrOfBytes(long size) { > > > + protected void incQueuedNrOfBytes(long size) { > > > queuedNrOfBytes += size; > > > } > > > > > > - protected synchronized void decQueuedNrOfBytes(long size) { > > > + protected void decQueuedNrOfBytes(long size) { > > > queuedNrOfBytes -= size; > > > } > > > > > > @@ -562,8 +569,10 @@ > > > .getKey()), x); > > > } > > > } finally { > > > - outQueueCounter++; > > > - decQueuedNrOfBytes(messagesize); > > > + synchronized (sender.mutex) { > > > + outQueueCounter++; > > > + > decQueuedNrOfBytes(messagesize); > > > + } > > > } > > > entry = entry.next(); > > > } while (keepRunning&& entry != null); > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
