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