Mailing-List: contact river-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: river-dev@incubator.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
Message-ID: <4CF38585.6040609@zeus.net.au>
Date: Mon, 29 Nov 2010 20:50:45 +1000
From: Peter Firmstone <jini@zeus.net.au>
User-Agent: Thunderbird 2.0.0.14 (X11/20080531)
MIME-Version: 1.0
To: river-dev@incubator.apache.org
Subject: Re: Possible multi-threading bug
References: <4CF2A8F2.9030701@acm.org>	<4CF2DD34.70506@zeus.net.au>
	<4CF2E2D2.8060803@acm.org>
 <AANLkTi=YEfL+PQHiLUkfde-yqd6vyt1ttcT-GTsPn9Co@mail.gmail.com>
In-Reply-To: <AANLkTi=YEfL+PQHiLUkfde-yqd6vyt1ttcT-GTsPn9Co@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Tom Hobbs wrote:
> Serious comment:
>
> Shouldn't reset also be synchronized?
You've identified that reset is also a mutator method, I guess I gave 
mutation as my reason and I wasn't being entirely accurate, sorry.

To explain a bit more, reset doesn't have to be synchronized when _count 
is volatile and increment synchronized, the reason, it's a single 
operation mutator, it just resets the counter, it doesn't check it first.

The increment method is at least two operations _count++ first gets 
_count, then increments it and sets _count to the new value, if it 
wasn't synchronized, another increment could get _count at the same 
time... Or a reset operation could occur in between and the reset fail.

The ++ operator isn't atomic.

If in doubt use synchronization, so by making reset also synchronized 
you wouldn't be wrong and it probably wouldn't hurt performance either.

The secret to concurrency is, all operations must happen atomically.  
Synchronization creates atomicity when it wouldn't otherwise exist.  
Synchronization also effects visibility, as does volatile.

Cheers,

Peter.

>   Also, +1 for making _count volatile.
>
> Frivolous comment:
>
> Can we drop the underscore prefix for member variables?  It's against
> the official Sun/Oracle/Whatever conventions.  Also, it drives me up
> the wall...
>
> http://www.oracle.com/technetwork/java/codeconventions-135099.html#367
>
>
> On Sun, Nov 28, 2010 at 11:16 PM, Patricia Shanahan <pats@acm.org> wrote:
>   
>> In the way in which this is used, I expect most of the calls to be to
>> increment. It has to be synchronized, so it seems simplest to synchronize
>> all the methods.
>>
>> I've done some more experiments, and decided this is a real problem. As part
>> of my debug effort, I increased the number of threads in the stress test, so
>> that it fails much more often. I also added some debug printouts, one of
>> which was showing up in conjunction with some but not all failures, so I
>> thought it was irrelevant.
>>
>> With the additional synchronization, the debug message shows up in all
>> failures. I think I actually had two forms of failure, one of which is fixed
>> by the synchronization.  In the failure case that has been fixed, everything
>> works, no debug messages, but the test never admits to having finished,
>> exactly the symptom I would expect from this issue.
>>
>> I plan to check in the enhanced test as well as the fixes, because it only
>> takes a few minutes longer than the current size, and is much better at
>> finding bugs.
>>
>> Patricia
>>
>>
>> On 11/28/2010 2:52 PM, Peter Firmstone wrote:
>>     
>>> Well, at the absolute minimum the variable should be volatile, so any
>>> changes are visible among all threads.
>>>
>>> Since increment is the only mutating method, this must be synchronized.
>>>
>>> This is a cheap form of multi read, single write threading, although one
>>> must be careful, as this only works on primitives and immutable object
>>> references, since only the reference itself is being updated.
>>>
>>> If it was a reference to a mutable object (or long), then all methods
>>> would need to be synchronized.
>>>
>>> Cheers,
>>>
>>> Peter.
>>>
>>> Patricia Shanahan wrote:
>>>       
>>>> I've found something I think is a problem in
>>>> com.sun.jini.test.impl.outrigger.matching.StressTest, but it does not
>>>> seem to be the problem, or at least not the only problem, causing the
>>>> test hang I'm investigating. It's difficult to test, so I'd like a
>>>> review of my reasoning. This is a question for those who are familiar
>>>> with the Java memory model.
>>>>
>>>> Incidentally, if we went to 1.5 as the oldest supported release, this
>>>> could be replaced by an AtomicInteger.
>>>>
>>>> In the following inner class, I think the methods reset and getCount
>>>> should be synchronized. As the comments note, the operations they
>>>> perform are atomic. My concern is the lack of a happens-before
>>>> relationship between those two methods and the increment method. As
>>>> far as I can tell, there is nothing forcing the change in the counter
>>>> due to an increment to become visible to a getCount call in another
>>>> thread.
>>>>
>>>> private class Counter {
>>>>
>>>> /**
>>>> * Internal counter variable.
>>>> */
>>>> private int _count = 0;
>>>>
>>>> /**
>>>> * Constructor. Declared to enforce protection level.
>>>> */
>>>> Counter() {
>>>>
>>>> // Do nothing.
>>>> }
>>>>
>>>> /**
>>>> * Resets internal counter to zero.
>>>> */
>>>> void reset() {
>>>>
>>>> // Integer assignment is atomic.
>>>> _count = 0;
>>>> }
>>>>
>>>> /**
>>>> * Increments internal counter by one.
>>>> */
>>>> synchronized void increment() {
>>>> ++_count;
>>>> }
>>>>
>>>> /**
>>>> * Returns current value of this <code>Counter</code> object.
>>>> */
>>>> int getCount() {
>>>>
>>>> // Returning an integer is atomic.
>>>> return _count;
>>>> }
>>>> }
>>>>
>>>>         
>>>       
>>     
>
>