On 15/09/2015 17:30, Caldarale, Charles R wrote: >> From: Yilong Li [mailto:yilong...@runtimeverification.com] >> Subject: Re: svn commit: r1703194 - >> /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java > >> I am not sure declaring this field as volatile is the right way to fix it >> because the increment is still not atomic. If this counter doesn't have to >> be precise, I think it's OK to allow data races on this field. Otherwise, >> it should be declared as atomic. > > Correct; marking the field as volatile serves no purpose here, since the > value does not need to be precise.
Using volatile does fix this issue. The odds of a data race here are slim (to non-existent) since updates from different threads will be at least 1s (possibly more I'd need to look at the code more closely) apart. The use of volatile ensures that the current thread updating the field sees the value set by the previous thread to update it. If updates were really concurrent then I'd agree volatile is not sufficient to fix this but that is not the case for this code. We could switch to atomic in case the way this code is used varies over time but I don't see that happening. Some further comments in the code t explain the above are probably called for. > Even making it atomic would not prevent multiple threads from iterating over > the map concurrently in the backgroundProess() method; the only ways to > prevent that are having the same lock held on the counter and the iteration > loop, or the same lock on the counter and a busy flag. With the current code that can't happen. There is only ever one background processor thread in existence at a time and it calls backgroundProcess() in the same thread. > I am concerned that the iterator in the for loop of backgroundProcess() may > lose its validity if another thread updates the map, but I'll need to study > the documentation and implementation to see if that's a real concern. > Regardless, using one of the forEach*() methods of ConcurrentHashMap would > probably be more efficient. The Javadoc for ConcurrentHashMap says this usage is safe. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
