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>: >>> kkoli...@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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
