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>: >>>> 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).
Yeah, something isn't quite right yet. I need to dig into this some more. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
