sync on local variable

[Daniel Pitts]

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.

I agree except, don't use toArray, use new ArrayList<Row>();
Or, instead of all the sync, check, blah-de-blah, use the standard EDT
mechanisms for modifying/accessing this table. That way, you're actions
are all serial, and you don't have to worry so much.

Threading isn't that hard to get right, but it isn't that hard to get
wrong either. The only truly difficult part of writing MT code is that
people aren't taught how to look at it and determine whether it is right
or not.

 

[Roedy Green]

It's very difficult to reason about locks and synchronization when
they depend on changeable references,

They don't. The two variables are final in the real code. I
"simplified". My bad.

I think the problem is just that in general local variables can be
subject to casual modification, perhaps during a later patch, which
would break the lock.

 

[Roedy Green]

The IntelliJ code Inspector (lint) slapped my wrist for synchronising
on a local variable. What's the problem with that? The sync is on
the object, not the reference, right?

I got this response from an IntelliJ employee (probably a Russian with
ESL).

The point was synchronization only makes sense if two different
threads syncing on the same instance and question arises how safe was
an exchange, that two threads have same reference on their stacks.
Most obviously "clean" subject to synchronize on is a final field.
Synchronizing on a local reference might well be not a problem but
generally not that easy to verify for correctness and easy thing to
broke later.

 

[Roedy Green]

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.

Sorry I thought I had pointed you to Vercheck.java.

It has since occurred to me I could try concocting a much simpler
piece of code without any JTable that still triggered the lint.

 

[Roedy Green]

Threading isn't that hard to get right, but it isn't that hard to get
wrong either. The only truly difficult part of writing MT code is that
people aren't taught how to look at it and determine whether it is right
or not.

The compiler rarely complains and the program nearly always run one
thread at a time. You can't easily simulate pathological cases the way
you can with single thread. Perhaps there is some drug that induces
paranoia that could be used once you have your first cut.

 

[markspace]

I got this response from an IntelliJ employee (probably a Russian with
ESL).

Hmm, well that's one strike against IntelliJ then, if they don't hire
tech support that can communicate properly.

 

[Arne Vajhøj]

I got this response from an IntelliJ employee (probably a Russian with
ESL).

The point was synchronization only makes sense if two different
threads syncing on the same instance and question arises how safe was
an exchange, that two threads have same reference on their stacks.
Most obviously "clean" subject to synchronize on is a final field.
Synchronizing on a local reference might well be not a problem but
generally not that easy to verify for correctness and easy thing to
broke later.

JetBrains is a czech company, so czech not russian sounds more
likely.

But the text looks fine to me. It is more or less what we
have been telling you for two days.

Arne

 

[Arne Vajhøj]

Threading isn't that hard to get right, but it isn't that hard to get
wrong either. The only truly difficult part of writing MT code is that
people aren't taught how to look at it and determine whether it is right
or not.

That applies to other things than MT!



Arne

 

[Tom Anderson]

The IntelliJ code Inspector (lint) slapped my wrist for synchronising on
a local variable. What's the problem with that? The sync is on the
object, not the reference, right?

It's like the warning on the lack of a serialVersionUID in a Serializable
class. It isn't necessarily wrong, but there is a popular class of errors
that involve doing this.

tom

 

[Eric Sosman]

>> [... about iterating over a changing List ...]
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.

>
I agree except, don't use toArray, use new ArrayList<Row>();

Not confrontational, just curious: Why prefer a new ArrayList
to an array? To me, it appears that an ArrayList is just an array
wrapped up in extra machinery, and I can't see that the machinery
adds any value for this usage. So, why pay the extra freight?
What am I missing?

 

[Daniel Pitts]

[... about iterating over a changing List ...]
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.

I agree except, don't use toArray, use new ArrayList<Row>();

Not confrontational, just curious: Why prefer a new ArrayList
to an array? To me, it appears that an ArrayList is just an array
wrapped up in extra machinery, and I can't see that the machinery
adds any value for this usage. So, why pay the extra freight?
What am I missing?

Array is a relatively primitive concept. You're question is akin to
asking why use a "Date object" instead of an "int representing the
milliseconds since Jan 1, 1970".

Either one "works". Yes, the non-primitive has more "machinery", but
that isn't automatically a bad thing. The primitive looses semantic
meaning outside of its context, where a properly designed abstraction
maintains its semantics regardless of context.

Lists are also easier to work with, and work in more places.

 

[Daniel Pitts]

I got this response from an IntelliJ employee (probably a Russian with
ESL).

The point was synchronization only makes sense if two different
threads syncing on the same instance and question arises how safe was
an exchange, that two threads have same reference on their stacks.
Most obviously "clean" subject to synchronize on is a final field.
Synchronizing on a local reference might well be not a problem but
generally not that easy to verify for correctness and easy thing to
broke later.

One thing you can do is more the synchronization into the "Row" class,
and turn your operations into atomic operations, rather than have
external clients need to concern themselves with synchronization.

That way, the Row class can always sync on the same object (probably
"this", but not necessarily), and the client needn't care about it.

 

[Arved Sandstrom]

On 3/25/2010 11:06 AM, Eric Sosman wrote:
[... about iterating over a changing List ...]
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.

I agree except, don't use toArray, use new ArrayList<Row>();

Not confrontational, just curious: Why prefer a new ArrayList
to an array? To me, it appears that an ArrayList is just an array
wrapped up in extra machinery, and I can't see that the machinery
adds any value for this usage. So, why pay the extra freight?
What am I missing?

Array is a relatively primitive concept. You're question is akin to
asking why use a "Date object" instead of an "int representing the
milliseconds since Jan 1, 1970".

Either one "works". Yes, the non-primitive has more "machinery", but
that isn't automatically a bad thing. The primitive looses semantic
meaning outside of its context, where a properly designed abstraction
maintains its semantics regardless of context.

Lists are also easier to work with, and work in more places.

I tend to use an array where the underlying structure I am representing
really is an array, and a collection where the underlying structure
really is a list or set or whatever.

Specifically I am not going to use an ArrayList, for example, if the
thing being modelled is not resizable. Why would I want to write code to
ensure that the ArrayList will stay at a fixed size when an aray will
already take care of that for me?

AHS

 

[Eric Sosman]

On 3/25/2010 11:06 AM, Eric Sosman wrote:
[... about iterating over a changing List ...]
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.

I agree except, don't use toArray, use new ArrayList<Row>();

Not confrontational, just curious: Why prefer a new ArrayList
to an array? To me, it appears that an ArrayList is just an array
wrapped up in extra machinery, and I can't see that the machinery
adds any value for this usage. So, why pay the extra freight?
What am I missing?

Array is a relatively primitive concept. You're question is akin to
asking why use a "Date object" instead of an "int representing the
milliseconds since Jan 1, 1970".

I think you've over-generalized my question. I'm not
asking whether arrays or Lists (longs or Dates) are preferable
in all circumstances, nor even in most circumstances. Rather,
I'm asking why you prefer the "heavier" object in the particular
situation Roedy faces.

Either one "works". Yes, the non-primitive has more "machinery", but
that isn't automatically a bad thing.

Okay. I'd also say it's not automatically a good thing,
especially when the machinery is not going to be used.

How much extra machinery are we talking about, in this specific
case? I'm suggesting a call to toArray() followed by an iteration
over the array: One new object created. A new ArrayList involves
creating that same array (perhaps twice, but I don't know whether
that happens here; there's a bug number I haven't looked up), creates
the ArrayList itself, and creates an Iterator when you actually do
the traversal. Three (four?) new objects instead of one.

How heavy are the extra objects? The ArrayList carries two
fields, plus another inherited from AbstractList. The Iterator
carries four more fields (you'll only see three in the source,
but inner classes carry a hidden reference to their owner).
During the iteration, the Iterator carefully checks whether the
ArrayList has been modified; we know it will not have been (the
reason we made the snapshot was so no modifications would disturb
us). Each item retrieved takes three bounds checks (nominally)
instead of one: One in hasNext(), one in next(), and one in the
actual array fetch. And so on.

Okay, okay, okay: Memory's cheap, CPU's are fast, only misers
count their change. But on the other hand, "Take care of the
pennies and the pounds will take care of themselves," and "Don't
use a cannon to kill a canary."

The primitive looses semantic
meaning outside of its context, where a properly designed abstraction
maintains its semantics regardless of context.

The comparison is between an array and an ArrayList. The
only semantic difference I see between them is that the latter
can grow and shrink, while the former cannot. But in the case
at hand, the goal is to get a snapshot that will remain unchanged,
that is, to avoid growth and shrinkage. Again, it seems to me
we're paying for capabilities that will not be used.

Lists are also easier to work with, and work in more places.

Once again, I return to the particular use in consideration.
The comparison is between

Row[] rows;
synchronized(ALL_ROWS) {
rows = ALL_ROWS.toArray(new Row[ALL_ROWS.size()]);
}
for (Row r : rows) { ... }

and

ArrayList<Row> rows;
synchronized(ALL_ROWS) {
rows = new ArrayList<Row>(ALL_ROWS);
}
for (Row r : rows) { ... }

There's only one "place" to consider, but still: Point to you
for being easier by five keystrokes. (With a simple change I
could get a fourteen-key swing and beat you by nine, but at the
cost of creating two arrays instead of one.)

Even though my first language was FORTRAN, I have no special
love for the array nor no special antipathy for the List. I'm
happy to use either. But I *do* have a preference for lighter-
weight gadgets when they're adequate for the purpose at hand, and
a Yankee's aversion to paying for unused extras. YM[*]MV.

[*] Motivation.

 

[Arne Vajhøj]

[... about iterating over a changing List ...]
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.

I agree except, don't use toArray, use new ArrayList<Row>();

Not confrontational, just curious: Why prefer a new ArrayList
to an array? To me, it appears that an ArrayList is just an array
wrapped up in extra machinery, and I can't see that the machinery
adds any value for this usage. So, why pay the extra freight?
What am I missing?

As a general rule:

know that more functionality than array will be needed in the future =>
pick ArrayList

know that more functionality than array will NOT be needed in the future
=> pick array

don't know if more functionality than array will be needed in the future
=> pick ArrayList

My assumption would be that in the real world that would be 10%-10%-80%.

Obviously you can argue that this seems fell in category #2.

Arne

 

[Daniel Pitts]

[...]
One thing you can do is more the synchronization into the "Row" class,
and turn your operations into atomic operations, rather than have
external clients need to concern themselves with synchronization.

That way, the Row class can always sync on the same object (probably
"this", but not necessarily), and the client needn't care about it.

Are you sure he can do that? He calls three methods on
the row instance, in two synchronized blocks. Split them into
three blocks, and the pair that were together may no longer see
their row in the same state, because something may happen after
one method releases the lock and before the next acquires it.

The first call is synchronized separately. The second and third call
are in one synchronized block. The "atomicity" of those two calls can
be handled in one method:

class MarkedUrl {
private final VersionURL url;
private final Marker marker;

// appropriate contstructor and getters.
}

public class AppToWatch {
public syncronized MarkedUrl getMarkedUrl() {
return new MarkedUrl(versionUrl, marker);
}
}