sync on local variable

[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?
--
Roedy Green Canadian Mind Products
http://mindprod.com

Don’t worry about people stealing an idea; if it’s original, you’ll have to shove it down their throats.
~ Howard Aiken (born: 1900-03-08 died: 1973-03-14 at age: 73)

 

[markspace]

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?

Need SSCCE.

http://sscce.org/

 

[Arved Sandstrom]

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?

Each thread gets its own local variable. Defeats the purpose of
synchronizing on one.

AHS

 

[Arne Vajhøj]

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?

Local variables are thread specific.

Passing on a local variable to another thread via some
global variable/singleton/cache would be a pretty bad design.

I can understand the complaint.

Arne

PS: yes - it is the object not the ref.

 

[Arne Vajhøj]

It may be a case of import, rather than export. Suppose the method for
obtaining a reference to some object is fairly expensive, and
synchronized. Each thread could have obtained a reference to the same
object and cached it in a local variable.

It could be.

But I would tend to believe that would be messy as well. To be able to
easily verify that threads are indeed synchronizing on the same object,
then I think it is best if is relative simple to see what is being
synchronized on.

It's difficult to know if the message was valid or not without an SSCCE.

I don't think the tool does super sophisticated code analysis. Most
likely it just have some simple rules and flag things that looks weird.
If they are good enough, then the developer can ignore the warning. And
a good tool would have the option to store the ignore decision.

Arne

 

[Mike Schilling]

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?

Right, it's not illegal and may be correct. But it may be incorrect as
well, and probably is will be unless you can guarantee that the local will
point to the same instance in all threads. IntelliJ thinks enough people
make that mistake that it's worth warning about. It's similar to

while (methodCall());
i++;

That's legal and might be exactly what you meant to do, but the first
semicolon is likely enough to be spurious that a lint-like tool should say
something.

 

[Arved Sandstrom]

If you later export that local reference to another thread or object, it
should be fine. It's valid to synchronize on a object that some other
part of the system will see later.

If you just synchronized on a local variable and then let it go out of
scope, yeah, that's a pretty serious "D'oh!" moment.

I think all the responses have pretty much narrowed in on the same
things: the sync is indeed on the object, which means that however you
obtain/store a reference to that object that everything is using as a
lock, it's certainly legal to sync on an object referred to locally.

But since it's something that is somewhat unusual, and could (probably)
be a mistake, I imagine IntelliJ is flagging it along the lines of what
Mike suggested.

AHS

 

[Roedy Green]

If you later export that local reference to another thread or object, it
should be fine. It's valid to synchronize on a object that some other
part of the system will see later.

I have a JTable. I get a row put it in a local variable and
synchronise on that. Does that not lock anyone else getting that
row, even if they have nothing to do with my local variable?

AppToWatch row;
synchronized ( ALL_ROWS )
{
row = ALL_ROWS.get( rowIndex );
state = row.getState();
}
....
synchronized ( row )
{
url = row.getVersionURL();
marker = row.getMarker();
}
--
Roedy Green Canadian Mind Products
http://mindprod.com

Don’t worry about people stealing an idea; if it’s original, you’ll have to shove it down their throats.
~ Howard Aiken (born: 1900-03-08 died: 1973-03-14 at age: 73)

 

[Arne Vajhøj]

I have a JTable. I get a row put it in a local variable and
synchronise on that. Does that not lock anyone else getting that
row, even if they have nothing to do with my local variable?

It does.

But the code would probably be difficult to read and this
is why a code style checker flag it.

Arne

 

[Eric Sosman]

I have a JTable. I get a row put it in a local variable and
synchronise on that. Does that not lock anyone else getting that
row, even if they have nothing to do with my local variable?

AppToWatch row;
synchronized ( ALL_ROWS )
{
row = ALL_ROWS.get( rowIndex );
state = row.getState();
}
...
synchronized ( row )
{
url = row.getVersionURL();
marker = row.getMarker();
}

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.

 

[Daniel Pitts]

I have a JTable. I get a row put it in a local variable and
synchronise on that. Does that not lock anyone else getting that
row, even if they have nothing to do with my local variable?

AppToWatch row;
synchronized ( ALL_ROWS )
{
row = ALL_ROWS.get( rowIndex );
state = row.getState();
}
....
synchronized ( row )
{
url = row.getVersionURL();
marker = row.getMarker();
}

I can't say for sure without seeing more of the code, but this looks
like a road to deadlock.

 

[Roedy Green]

Are you sure you didn't misread IntelliJ's complaint?

all it says is "synchronization on local variable row".

 

[Roedy Green]

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.

I corrected the code to read:

AppToWatch row;
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.

 

[Roedy Green]

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.

 

[Eric Sosman]

[...]
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.

 

[Lew]

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

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.

 

[Eric Sosman]

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.

 

[Roedy Green]

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.

 

[Wojtek]

Roedy Green wrote :

I corrected the code to read:

Have you tried to explicitly initialise row?

AppToWatch row = null;

 

[Daniel Pitts]

The problem is any piece of code with a JTable in it and TableModel is
still going to be obese.

Do you believe your problem is because of those classes? If not, you can
write an SSCCE that doesn't use them.