You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/09/15 15:43:55 UTC
svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Author: markt
Date: Tue Sep 15 13:43:55 2015
New Revision: 1703194
URL: http://svn.apache.org/r1703194
Log:
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=58384
Calls to backgroundProcess() may come from different threads
Modified:
tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Modified: tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java?rev=1703194&r1=1703193&r2=1703194&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java Tue Sep 15 13:43:55 2015
@@ -92,7 +92,7 @@ public class WsWebSocketContainer implem
private int maxBinaryMessageBufferSize = Constants.DEFAULT_BUFFER_SIZE;
private int maxTextMessageBufferSize = Constants.DEFAULT_BUFFER_SIZE;
private volatile long defaultMaxSessionIdleTimeout = 0;
- private int backgroundProcessCount = 0;
+ private volatile int backgroundProcessCount = 0;
private int processPeriod = Constants.DEFAULT_PROCESS_PERIOD;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 15/09/2015 21:42, Rémy Maucherat wrote:
> 2015-09-15 22:28 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
>>> Sounds like a false positive to me. There are numerous implicit
>> synchronizations in the above sequence, so the volatile is not particularly
>> useful (but it's not terribly expensive either).
>>
>> Thanks. I'll revert that fix and resolve it as invalid.
>>
> OTOH since you're in that section, the TCK prefers if the background check
> for session expiration is run once per second. How epensive is it ? It
> would make that algorithm a lot simpler.
It has to iterate through every single connection. The check shouldn't
be that expensive and it looks like there are a few optimisations we
could apply if necessary.
The most expensive part is sending the close messages (which uses
blocking I/O). I suspect that at scale this will become a bottleneck.
Some form of dispatch to a separate thread will probably be required.
That is likely to add more complexity than will be removed if we go to a
check every second.
I'm wary of premature optimisation here. We probably need to come up
with some sort of load test (probably one that emulates slow clients) to
figure out how much of an issue this is.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1703194 - /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-09-15 22:28 GMT+02:00 Mark Thomas <ma...@apache.org>:
> > Sounds like a false positive to me. There are numerous implicit
> synchronizations in the above sequence, so the volatile is not particularly
> useful (but it's not terribly expensive either).
>
> Thanks. I'll revert that fix and resolve it as invalid.
>
> OTOH since you're in that section, the TCK prefers if the background check
for session expiration is run once per second. How epensive is it ? It
would make that algorithm a lot simpler.
Rémy
Re: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 15/09/2015 21:24, Caldarale, Charles R wrote:
>> From: Mark Thomas [mailto:markt@apache.org]
>> Subject: Re: svn commit: r1703194 - /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
>
>> What prevents the concurrency is that there is only ever one background
>> thread. The only was I see this happening is:
>> - T1 runs and expires all the current sessions.
>> - The container unregisters itself from background processing since it
>> has no sessions
>> - It was the only thing registered for background processing so the
>> T1 is terminated by the BackgroundProcessManager
>> - A new WeBSoket session is opened
>> - The container registers itself with the BackgroundProcessManager
>> - A new background processing thread T2 is started
>> - T2 runs
>
>> RV-Predict indicated that there was a race. Was this a false positive?
>
> Sounds like a false positive to me. There are numerous implicit synchronizations in the above sequence, so the volatile is not particularly useful (but it's not terribly expensive either).
Thanks. I'll revert that fix and resolve it as invalid.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Mark Thomas [mailto:markt@apache.org]
> Subject: Re: svn commit: r1703194 - /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
> What prevents the concurrency is that there is only ever one background
> thread. The only was I see this happening is:
> - T1 runs and expires all the current sessions.
> - The container unregisters itself from background processing since it
> has no sessions
> - It was the only thing registered for background processing so the
> T1 is terminated by the BackgroundProcessManager
> - A new WeBSoket session is opened
> - The container registers itself with the BackgroundProcessManager
> - A new background processing thread T2 is started
> - T2 runs
> RV-Predict indicated that there was a race. Was this a false positive?
Sounds like a false positive to me. There are numerous implicit synchronizations in the above sequence, so the volatile is not particularly useful (but it's not terribly expensive either).
> I was looking at keyset
> "they are guaranteed to traverse elements as they existed upon
> construction exactly once, and may (but are not guaranteed to) reflect
> any modifications subsequent to construction."
Ok, finally found the above under java/util/concurrent/package-summary.html#Weakly, so that seems to be ok.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 15/09/2015 18:35, Caldarale, Charles R wrote:
>> From: Mark Thomas [mailto:markt@apache.org]
>> Subject: Re: svn commit: r1703194 -
>> /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
>
>> The odds of a data race here are slim (to non-existent)
>
> That's definitely true; the questions arising here are more theoretical than practical for this particular instance. However, they may be applicable to other areas.
>
>> The use of volatile ensures that the current thread updating the
>> field sees the value set by the previous thread to update it.
>
> Ok, I misunderstood the situation - didn't realize that something else is preventing actual concurrent use. But since the whole issue is based on multiple threads calling this method, what prevents the concurrency? (I've seen threads on Linux interrupted and held up for seconds when unrelated page table management is underway, allowing some very unexpected data sharing.) If the multiple threads that arrive here sequentially are ordered by some other mechanism (e.g., synchronization, context switch, termination), the volatile still isn't needed.
What prevents the concurrency is that there is only ever one background
thread. The only was I see this happening is:
- T1 runs and expires all the current sessions.
- The container unregisters itself from background processing since it
has no sessions
- It was the only thing registered for background processing so the
T1 is terminated by the BackgroundProcessManager
- A new WeBSoket session is opened
- The container registers itself with the BackgroundProcessManager
- A new background processing thread T2 is started
- T2 runs
RV-Predict indicated that there was a race. Was this a false positive?
The overhead associated with using volatile is minimal but I'd be happy
to drop it if it is not necessary.
>> Some further comments in the code t explain the above are probably
>> called for.
>
> That would indeed help.
>
>>> I am concerned that the iterator in the for loop of backgroundProcess() may lose its
>>> validity if another thread updates the map
>
>> The Javadoc for ConcurrentHashMap says this usage is safe.
>
> The only statement I can find in the Javadoc is this:
>
> "Similarly, Iterators, Spliterators and Enumerations return elements reflecting the state of the hash table at some point at or since the creation of the iterator/enumeration. They do not throw ConcurrentModificationException."
>
> Not quite the reassurance I'm looking for that an iterator will actually be useful (i.e., find all remaining entries) after the map has been mutated and possibly reorganized by another thread. This is one spot where the C++ spec is clearer (rather surprisingly).
I was looking at keyset
"they are guaranteed to traverse elements as they existed upon
construction exactly once, and may (but are not guaranteed to) reflect
any modifications subsequent to construction."
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Mark Thomas [mailto:markt@apache.org]
> Subject: Re: svn commit: r1703194 -
> /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
> The odds of a data race here are slim (to non-existent)
That's definitely true; the questions arising here are more theoretical than practical for this particular instance. However, they may be applicable to other areas.
> The use of volatile ensures that the current thread updating the
> field sees the value set by the previous thread to update it.
Ok, I misunderstood the situation - didn't realize that something else is preventing actual concurrent use. But since the whole issue is based on multiple threads calling this method, what prevents the concurrency? (I've seen threads on Linux interrupted and held up for seconds when unrelated page table management is underway, allowing some very unexpected data sharing.) If the multiple threads that arrive here sequentially are ordered by some other mechanism (e.g., synchronization, context switch, termination), the volatile still isn't needed.
> Some further comments in the code t explain the above are probably
> called for.
That would indeed help.
> > I am concerned that the iterator in the for loop of backgroundProcess() may lose its
> > validity if another thread updates the map
> The Javadoc for ConcurrentHashMap says this usage is safe.
The only statement I can find in the Javadoc is this:
"Similarly, Iterators, Spliterators and Enumerations return elements reflecting the state of the hash table at some point at or since the creation of the iterator/enumeration. They do not throw ConcurrentModificationException."
Not quite the reassurance I'm looking for that an iterator will actually be useful (i.e., find all remaining entries) after the map has been mutated and possibly reorganized by another thread. This is one spot where the C++ spec is clearer (rather surprisingly).
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 15/09/2015 17:30, Caldarale, Charles R wrote:
>> From: Yilong Li [mailto:yilong.li@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1703194 -
/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Yilong Li [mailto:yilong.li@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. 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.
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.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1703194 - /tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
Posted by Yilong Li <yi...@runtimeverification.com>.
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.
Yilong
On Tue, Sep 15, 2015 at 6:43 AM, <ma...@apache.org> wrote:
> Author: markt
> Date: Tue Sep 15 13:43:55 2015
> New Revision: 1703194
>
> URL: http://svn.apache.org/r1703194
> Log:
> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=58384
> Calls to backgroundProcess() may come from different threads
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java?rev=1703194&r1=1703193&r2=1703194&view=diff
>
> ==============================================================================
> ---
> tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
> (original)
> +++
> tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java Tue
> Sep 15 13:43:55 2015
> @@ -92,7 +92,7 @@ public class WsWebSocketContainer implem
> private int maxBinaryMessageBufferSize =
> Constants.DEFAULT_BUFFER_SIZE;
> private int maxTextMessageBufferSize = Constants.DEFAULT_BUFFER_SIZE;
> private volatile long defaultMaxSessionIdleTimeout = 0;
> - private int backgroundProcessCount = 0;
> + private volatile int backgroundProcessCount = 0;
> private int processPeriod = Constants.DEFAULT_PROCESS_PERIOD;
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>