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 <[email protected]> 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; >>> } >>> } >>> >> >> > >
