You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2013/01/25 23:34:57 UTC
svn commit: r1438747 -
/tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Author: kkolinko
Date: Fri Jan 25 22:34:57 2013
New Revision: 1438747
URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
Log:
Make the messages list synchronized as a whole, instead of just using a volatile reference to it.
I am still observing random failures with TestWsWebSocketContainer, so an issue is not here.
Modified:
tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Modified: tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java?rev=1438747&r1=1438746&r2=1438747&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java (original)
+++ tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java Fri Jan 25 22:34:57 2013
@@ -20,6 +20,7 @@ import java.io.IOException;
import java.net.URI;
import java.nio.ByteBuffer;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
@@ -245,7 +246,8 @@ public class TestWsWebSocketContainer ex
private final CountDownLatch latch;
- private volatile List<T> messages = new ArrayList<>();
+ private final List<T> messages = Collections
+ .synchronizedList(new ArrayList<T>());
public TesterMessageHandler(CountDownLatch latch) {
this.latch = latch;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1438747 - /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 28/01/2013 13:35, Konstantin Kolinko wrote:
> 2013/1/28 Mark Thomas <ma...@apache.org>:
>> On 26/01/2013 12:30, Konstantin Kolinko wrote:
>>> 2013/1/26 Mark Thomas <ma...@apache.org>:
>>>> kkolinko@apache.org wrote:
>>>>
>>>>> Author: kkolinko
>>>>> Date: Fri Jan 25 22:34:57 2013
>>>>> New Revision: 1438747
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
>>>>> Log:
>>>>> Make the messages list synchronized as a whole, instead of just using a
>>>>> volatile reference to it.
>>>>> I am still observing random failures with TestWsWebSocketContainer, so
>>>>> an issue is not here.
>>>>
>>>> So why make the change?
>>>>
>>>
>>> Because of
>>> http://svn.apache.org/viewvc?view=revision&revision=1437930
>>
>> OK. The log message seemed to suggest the change was pointless. Hence my
>> question.
>>
>>> If you just need to propagate the "messages" reference across threads,
>>> then I think r1437930 should have been more simple: just marking the
>>> field as "final".
>>> If you are protecting access to inner structures of ArrayList class, I
>>> think that marking the reference as volatile is not enough and that
>>> using a synchronized list is more adequate.
>>>
>>> I think "volatile" modifier applies to accessing the filed itself, and
>>> has no effect when you use the value returned by getMessages() or
>>> store it in a local variable.
>>>
>>> Well, I do not see where a concurrency can come from here, so mere
>>> "final" should be enough. The latch did count down (otherwise
>>> assertTrue(latchResult) detected a failure) so either a message was
>>> received or TesterEndpoint.onClose() or TesterEndpoint.onError() was
>>> called. The latch should provide synchronization between threads.
>>
>> Having just read up on "happens-before" I do not believe the list needs
>> to be synchronised.
>
> Agreed.
>
> I am OK with leaving it as is (as well as with CopyOnWriteArrayList
> from r1439351). It should not matter for this test case.
>
>> I believe the root cause of the problem I was trying
>> to solve was fixed in r1437930 when I switched the order of adding the
>> message to the list and counting down the latch.
>
> Agreed. It should be it.
> (For the record, you obviously meant r1438229 and the change in
> onMessage(..) in TestWsWebSocketContainer.java)
>
> BTW, the test still fails for BIO in the last two buildbot runs. (NIO was OK).
Yeah, something isn't quite right yet. I need to dig into this some more.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1438747 - /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/1/28 Mark Thomas <ma...@apache.org>:
> On 26/01/2013 12:30, Konstantin Kolinko wrote:
>> 2013/1/26 Mark Thomas <ma...@apache.org>:
>>> kkolinko@apache.org wrote:
>>>
>>>> Author: kkolinko
>>>> Date: Fri Jan 25 22:34:57 2013
>>>> New Revision: 1438747
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
>>>> Log:
>>>> Make the messages list synchronized as a whole, instead of just using a
>>>> volatile reference to it.
>>>> I am still observing random failures with TestWsWebSocketContainer, so
>>>> an issue is not here.
>>>
>>> So why make the change?
>>>
>>
>> Because of
>> http://svn.apache.org/viewvc?view=revision&revision=1437930
>
> OK. The log message seemed to suggest the change was pointless. Hence my
> question.
>
>> If you just need to propagate the "messages" reference across threads,
>> then I think r1437930 should have been more simple: just marking the
>> field as "final".
>> If you are protecting access to inner structures of ArrayList class, I
>> think that marking the reference as volatile is not enough and that
>> using a synchronized list is more adequate.
>>
>> I think "volatile" modifier applies to accessing the filed itself, and
>> has no effect when you use the value returned by getMessages() or
>> store it in a local variable.
>>
>> Well, I do not see where a concurrency can come from here, so mere
>> "final" should be enough. The latch did count down (otherwise
>> assertTrue(latchResult) detected a failure) so either a message was
>> received or TesterEndpoint.onClose() or TesterEndpoint.onError() was
>> called. The latch should provide synchronization between threads.
>
> Having just read up on "happens-before" I do not believe the list needs
> to be synchronised.
Agreed.
I am OK with leaving it as is (as well as with CopyOnWriteArrayList
from r1439351). It should not matter for this test case.
> I believe the root cause of the problem I was trying
> to solve was fixed in r1437930 when I switched the order of adding the
> message to the list and counting down the latch.
Agreed. It should be it.
(For the record, you obviously meant r1438229 and the change in
onMessage(..) in TestWsWebSocketContainer.java)
BTW, the test still fails for BIO in the last two buildbot runs. (NIO was OK).
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1438747 - /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
On 26/01/2013 12:30, Konstantin Kolinko wrote:
> 2013/1/26 Mark Thomas <ma...@apache.org>:
>> kkolinko@apache.org wrote:
>>
>>> Author: kkolinko
>>> Date: Fri Jan 25 22:34:57 2013
>>> New Revision: 1438747
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
>>> Log:
>>> Make the messages list synchronized as a whole, instead of just using a
>>> volatile reference to it.
>>> I am still observing random failures with TestWsWebSocketContainer, so
>>> an issue is not here.
>>
>> So why make the change?
>>
>
> Because of
> http://svn.apache.org/viewvc?view=revision&revision=1437930
OK. The log message seemed to suggest the change was pointless. Hence my
question.
> If you just need to propagate the "messages" reference across threads,
> then I think r1437930 should have been more simple: just marking the
> field as "final".
> If you are protecting access to inner structures of ArrayList class, I
> think that marking the reference as volatile is not enough and that
> using a synchronized list is more adequate.
>
> I think "volatile" modifier applies to accessing the filed itself, and
> has no effect when you use the value returned by getMessages() or
> store it in a local variable.
>
> Well, I do not see where a concurrency can come from here, so mere
> "final" should be enough. The latch did count down (otherwise
> assertTrue(latchResult) detected a failure) so either a message was
> received or TesterEndpoint.onClose() or TesterEndpoint.onError() was
> called. The latch should provide synchronization between threads.
Having just read up on "happens-before" I do not believe the list needs
to be synchronised. I believe the root cause of the problem I was trying
to solve was fixed in r1437930 when I switched the order of adding the
message to the list and counting down the latch.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1438747 - /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/1/26 Mark Thomas <ma...@apache.org>:
> kkolinko@apache.org wrote:
>
>>Author: kkolinko
>>Date: Fri Jan 25 22:34:57 2013
>>New Revision: 1438747
>>
>>URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
>>Log:
>>Make the messages list synchronized as a whole, instead of just using a
>>volatile reference to it.
>>I am still observing random failures with TestWsWebSocketContainer, so
>>an issue is not here.
>
> So why make the change?
>
Because of
http://svn.apache.org/viewvc?view=revision&revision=1437930
If you just need to propagate the "messages" reference across threads,
then I think r1437930 should have been more simple: just marking the
field as "final".
If you are protecting access to inner structures of ArrayList class, I
think that marking the reference as volatile is not enough and that
using a synchronized list is more adequate.
I think "volatile" modifier applies to accessing the filed itself, and
has no effect when you use the value returned by getMessages() or
store it in a local variable.
Well, I do not see where a concurrency can come from here, so mere
"final" should be enough. The latch did count down (otherwise
assertTrue(latchResult) detected a failure) so either a message was
received or TesterEndpoint.onClose() or TesterEndpoint.onError() was
called. The latch should provide synchronization between threads.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1438747 - /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
Posted by Mark Thomas <ma...@apache.org>.
kkolinko@apache.org wrote:
>Author: kkolinko
>Date: Fri Jan 25 22:34:57 2013
>New Revision: 1438747
>
>URL: http://svn.apache.org/viewvc?rev=1438747&view=rev
>Log:
>Make the messages list synchronized as a whole, instead of just using a
>volatile reference to it.
>I am still observing random failures with TestWsWebSocketContainer, so
>an issue is not here.
So why make the change?
Mark
>
>Modified:
>tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
>
>Modified:
>tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
>URL:
>http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java?rev=1438747&r1=1438746&r2=1438747&view=diff
>==============================================================================
>---
>tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
>(original)
>+++
>tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
>Fri Jan 25 22:34:57 2013
>@@ -20,6 +20,7 @@ import java.io.IOException;
> import java.net.URI;
> import java.nio.ByteBuffer;
> import java.util.ArrayList;
>+import java.util.Collections;
> import java.util.List;
> import java.util.concurrent.CountDownLatch;
> import java.util.concurrent.TimeUnit;
>@@ -245,7 +246,8 @@ public class TestWsWebSocketContainer ex
>
> private final CountDownLatch latch;
>
>- private volatile List<T> messages = new ArrayList<>();
>+ private final List<T> messages = Collections
>+ .synchronizedList(new ArrayList<T>());
>
> public TesterMessageHandler(CountDownLatch latch) {
> this.latch = latch;
>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org