Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Message-ID: <510660AF.9020104@apache.org>
Date: Mon, 28 Jan 2013 11:27:43 +0000
From: Mark Thomas <markt@apache.org>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:17.0) Gecko/20130107 Thunderbird/17.0.2
MIME-Version: 1.0
To: Tomcat Developers List <dev@tomcat.apache.org>
Subject: Re: svn commit: r1438747 -
 /tomcat/trunk/test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
References: <20130125223458.1413F23888E7@eris.apache.org>
 <cfdb4ef6-fae5-4b0a-a1ee-4b8197ef8066@email.android.com>
 <CABzHfVm-gYbRBja1EqeokK80xVYM22pwcGzfBNGd39hB5BxM5w@mail.gmail.com>
In-Reply-To: 
 <CABzHfVm-gYbRBja1EqeokK80xVYM22pwcGzfBNGd39hB5BxM5w@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

On 26/01/2013 12:30, Konstantin Kolinko wrote:
> 2013/1/26 Mark Thomas <markt@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