river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Firmstone <j...@zeus.net.au>
Subject Re: Possible multi-threading bug
Date Mon, 29 Nov 2010 10:52:44 GMT
+1 Peter.

Patricia Shanahan wrote:
> As I said in the original e-mail "I think the methods reset and getCount
> should be synchronized.".
>
> Either synchronizing both those methods or making _count volatile would
> fix the problem. I don't think performance matters on this class, so the
> simplest solution should win. The increment method is, and has to be,
> synchronized, so we might was well apply the same solution throughout.
>
> If performance did matter, I would still prefer making all the methods
> synchronized. Almost all calls are to increment. Making _count volatile
> would tend to slow down every call to increment by adding memory
> barriers. Making the other methods synchronized only affects increment
> in the rare case of contention with one of the others.
>
> As I've worked on the other problem, I've continued to build empirical
> evidence that this is a real problem. Since I made the changes to
> Counter, I have run the test dozens of times, without seeing the failure
> mode in which everything works but the test fails to notice it has
> finished. Before the fix, that was the commoner failure mode.
>
> The _count variable is part of a much bigger issue. I've been assuming
> that I should not make style fixes as part of bug fixes. Although I
> would have called the variable "count" myself, at least it does not get
> in the way of being able to read the code. I have more trouble with
> inconsistent indentation, to the extent that I sometimes reformat files
> I know I will revert before the next check-in.
>
> Maybe the roadmap should include a minor release at which we just fix
> style, including indentation?
>
> Patricia
>
>
> On 11/29/2010 1:55 AM, Tom Hobbs wrote:
>> Serious comment:
>>
>> Shouldn't reset also be synchronized?  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;
>>>>> }
>>>>> }
>>>>>
>>>>
>>>>
>>>
>>>
>>
>
>


Mime
View raw message