sync on local variable
in [Java Help]
|
From: Eric Sosman on 25 Mar 2010 13:38 On 3/25/2010 1:27 PM, Roedy Green wrote: > [...] > Now I get TWO message about synchorizing on row. A politician would call this "Progress." ;-) Sorry; I'm out of ideas about what IntelliJ dislikes. Let us know if (no, let's be optimistic, "when") you find out; it'll probably be interesting and/or instructive. -- Eric Sosman esosman(a)ieee-dot-org.invalid
From: Lew on 25 Mar 2010 14:01 Roedy Green wrote: > it is a bit fat for a SSCCE. Well, the point of an SSCCE is that you trim down the "fat" code to a minimal case that illustrates the problem. Part of the value in doing that is that you often find the problem in the course of doing the trimming. Eric Sosman >> Are you sure you didn't misread IntelliJ's complaint? The >> thing that strikes me as odd about this code is that there are >> three method calls made on row's object, only two of which are >> synchronized on that object. It's conceivable that this is all >> right, but it sure looks strange to me -- and perhaps IntelliJ >> thinks it peculiar, too. > Roedy Green wrote: > I corrected the code to read: > > AppToWatch row; > synchronized ( ALL_ROWS ) > { > row = ALL_ROWS.get( rowIndex ); > synchronized ( row ) > { > state = row.getState(); > } > } > ... It's not clear to me how this next section gets its 'row' reference. Presumably it has to synchronize on 'ALL_ROWS' via the above fragment. Regardless, any time you have inconsistent locking protocols, e.g., using two locks in one place and only one in another, you risk having strange problems, so it makes sense that IntelliJ warns you about it. As with "unchecked" warnings, that doesn't mean that you're wrong, necessarily, only that you're in dangerous territory. > synchronized ( row ) > { > url = row.getVersionURL(); > marker = row.getMarker(); > } > > Now I get TWO message about synchorizing on row. > It's very difficult to reason about locks and synchronization when they depend on changeable references, and involve different locking sequences. I swan the warning is appropriate and you should either live with it or come up with a locking protocol that's easier to understand. -- Lew
From: Eric Sosman on 25 Mar 2010 14:06 On 3/25/2010 1:29 PM, Roedy Green wrote: > On Wed, 24 Mar 2010 16:21:29 -0700, Patricia Shanahan<pats(a)acm.org> > wrote, quoted or indirectly quoted someone who said : > >> It's difficult to know if the message was valid or not without an SSCCE. > > you can see the complete code at > https://wush.net/websvn/mindprod/listing.php?repname=mindprod&path=/com/mindprod/vercheck/ > > it is a bit fat for a SSCCE. Would have been nice of you to point out which of the twelve source files elicited the complaint ... For others who may want a look, VerCheck.java seems to be the culprit. Roedy, I still don't know what's going on, but there's at least one synchronization problem apparent in the code: for ( int rowIndex = 0; rowIndex < ALL_ROWS.size(); rowIndex++ ) ... synchronized ( ALL_ROWS ) { row = ALL_ROWS.get( rowIndex ); As the comment just before this loop says, rows might be deleted from ALL_ROWS while this is running, meaning that rowIndex might be out of range by the time you call get(). This doesn't appear closely related to a complaint about synchronizing on row -- but I've often found, when confronted by a mysterious problem, that it pays to clean up *all* the other issues, even those that are "obviously unrelated." By the way, if ALL_ROWS can be perturbed while the loop is in progress, the iteration may miss rows (visit row 4, other thread deletes row 2 and slides everything down one slot, visit row 5 which used to be row 6, never visit the old row 5), or may visit a row more than once (visit row 4, other thread inserts a row after row 2 and slides everything up, visit row 5 which is the same row just visited as 4). The comment indicates you don't want to lock ALL_ROWS for the entire loop, but to keep the iteration self-consistent you might want to do something like lock ALL_ROWS, grab a snapshot with toArray(), unlock, and run the iteration on the (private, stable) snapshot. -- Eric Sosman esosman(a)ieee-dot-org.invalid
From: Roedy Green on 25 Mar 2010 14:35 On Thu, 25 Mar 2010 11:01:30 -0700 (PDT), Lew <lew(a)lewscanon.com> wrote, quoted or indirectly quoted someone who said : >> it is a bit fat for a SSCCE. > >Well, the point of an SSCCE is that you trim down the "fat" code to a >minimal case that illustrates the problem. Part of the value in doing >that is that you often find the problem in the course of doing the >trimming. The problem is any piece of code with a JTable in it and TableModel is still going to be obese. -- Roedy Green Canadian Mind Products http://mindprod.com If you tell a computer the same fact in more than one place, unless you have an automated mechanism to ensure they stay in sync, the versions of the fact will eventually get out of sync.
From: Wojtek on 25 Mar 2010 14:48
Roedy Green wrote : > On Thu, 25 Mar 2010 00:06:56 -0400, Eric Sosman > <esosman(a)ieee-dot-org.invalid> wrote, quoted or indirectly quoted > someone who said : > > I corrected the code to read: Have you tried to explicitly initialise row? AppToWatch row = null; > synchronized ( ALL_ROWS ) > { > row = ALL_ROWS.get( rowIndex ); > synchronized ( row ) > { > state = row.getState(); > } > } > ... > synchronized ( row ) > { > url = row.getVersionURL(); > marker = row.getMarker(); > } > > Now I get TWO message about synchorizing on row. -- Wojtek :-) |