Bug 52355 - Synchronize possible concurrent accesses to field "org.apache.catalina.tribes.transport.bio.util.FastQueue.checkLock".
Summary: Synchronize possible concurrent accesses to field "org.apache.catalina.tribes...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-18 23:34 UTC by Mohsen Vakilian
Modified: 2011-12-24 08:22 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mohsen Vakilian 2011-12-18 23:34:54 UTC
Class "org.apache.catalina.tribes.transport.bio.util.FastQueue" is used in parallel contexts that originate from method org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.run().

1. org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.run()
2. org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.sendAsyncData(LinkObject)
3. org.apache.catalina.tribes.group.ChannelInterceptorBase.sendMessage(Member[], ChannelMessage, InterceptorPayload)
4. org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.sendMessage(Member[], ChannelMessage, InterceptorPayload)
5. org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.addToQueue(ChannelMessage, Member[], InterceptorPayload)
6. org.apache.catalina.tribes.transport.bio.util.FastQueue.FastQueue()

Two public methods of "FastQueue", namely "org.apache.catalina.tribes.transport.bio.util.FastQueue.isCheckLock()" and "org.apache.catalina.tribes.transport.bio.util.FastQueue.setCheckLock(boolean)" access the field "org.apache.catalina.tribes.transport.bio.util.FastQueue.checkLock" without proper synchronization. Fortunately, these two methods, i.e. "FastQueue.isCheckLock()" and "FastQueue.setCheckLock(boolean)", are currently used. So, there is only a potential data race on "FastQueue.checkLock". To prevent such a data race in future, I suggest that either accesses to "FastQueue.checkLock" get properly syncrhonized or the unused public methods "FastQueue.isCheckLock()" and "FastQueue.setCheckLock(boolean)" be removed.


Field "FastQueue.checkLock" is declared at
<http://svn.apache.org/repos/asf/!svn/bc/1220560/tomcat/trunk/java/org/apache/catalina/tribes/transport/bio/util/FastQueue.java>.
Comment 1 Mark Thomas 2011-12-23 21:09:23 UTC
That is all essentially debug code. The setter may be accessed via reflection when server.xml is parsed by I haven't checked if it is exposed. I suspect that it is not exposed and that it is just changed in the source when debugging issues.

I'm not concerned about concurrent access but to be on the safe side it should probably be volatile. The flags it then uses to track status during debugging should be certainly be volatile.
Comment 2 Mark Thomas 2011-12-23 21:16:57 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.24 onwards.
Comment 3 Mohsen Vakilian 2011-12-24 00:09:18 UTC
Thanks for resolving this issue. Keshmesh <http://keshmesh.cs.illinois.edu/> detected this problem when we ran it on Tomcat using method "org.apache.catalina.tribes.group.interceptors.MessageDispatchInterceptor.run()" as an entry point.

(In reply to comment #2)
> Fixed in trunk and 7.0.x and will be included in 7.0.24 onwards.
Comment 4 Rainer Jung 2011-12-24 00:24:29 UTC
When I wrote that queue at the end of 2004 using some hand made asymmetric locks I added the checkLock flag to be able to debug in case the lock constructs would turn out to be buggy.

Now, 7 years later, ans after only very few changes to that code, I think we actually no longer need "checkLock" and the flags only neded in combination with checkLock, i.e. inAdd, inRemove, inMutex.

IMHO we can remove that part.
Comment 5 Mark Thomas 2011-12-24 08:22:03 UTC
You know me, always happy to delete code :)

Mark