river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tom Hobbs <tvho...@googlemail.com>
Subject Re: Possible multi-threading bug
Date Mon, 29 Nov 2010 10:54:05 GMT
> As I said in the original e-mail "I think the methods reset and getCount
> should be synchronized.".

Yes, you're right.  Sorry, my mistake.

I've just finished "Java Concurrency in Practice" which means I can
remember just enough to be dangerous, but not quite enough to
fool-proof.  "volatile" is referred to as a 'poor man synchronisation'
and is no means perfect in all cases.  Synchronizing the methods makes
this less-difficult to make mistakes with, so given that performance
isn't an issue I'd got for that rather than synchronizing on _count.

Regarding format and layout, I agree that the poor formatting makes
reading source files far more difficult than the odd underscore
creeping in.  My approach is that when I come across such a file,
before I do anything else to it, I reformat and commit the file back
in with a "reformat" comment.  Most diff tools can be convinced to
ignore whitespace when comparing different versions.  I find the only
time changing the layout becomes a problem is when a single commit
contains both layout and code changes.

That's just my opinion though.

On Mon, Nov 29, 2010 at 10:45 AM, Patricia Shanahan <pats@acm.org> 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