Is the @ThreadSafe annotation from JCIP incompatible with callbacks?

[neuneudr]

Hi,

if you want every method of a class to be synchronized, you
can use the @ThreadSafe annotation from JCIP and then use
FindBugs to find violations (at least that's the theory

However I'm wondering, when you have callbacks, those callbacks
are basically alien method calls and hence you cannot synchronize
every method?

So the following is incorrect because there's an alien method call:

public void synchronized doSomeStuff() {
...
if ( statechanged ) {
notifyObservers(); // alien method call
}
}


I have to write:

public void doSomeStuff() {
synchronized(this) {
...
}
if ( statechanged ) {
notifyObservers();
}
}

And then it means as soon as I've got callback I cannot
use the @ThreadSafe annotations from JCIP?

 

[Lew]

if you want every method of a class to be synchronized, you
can use the @ThreadSafe annotation from JCIP and then use
FindBugs to find violations (at least that's the theory

Making every method of a class synchronized doesn't necessarily make the class
thread safe.

You don't need the annotation to make every method synchronized.

However I'm wondering, when you have callbacks, those callbacks
are basically alien method calls and hence you cannot synchronize
every method?

Do you mean callbacks that the synchronized class calls, or callbacks that the
class donates to other class instances to call?

So the following is incorrect because there's an alien method call:

public void synchronized doSomeStuff() {
...
if ( statechanged ) {
notifyObservers(); // alien method call
}
}

There's not enough information here to determine if the method is correct, or
even what "correct" means for this class.

I have to write:

public void doSomeStuff() {
synchronized(this) {
...
}
if ( statechanged ) {
notifyObservers();
}
}

And then it means as soon as I've got callback I cannot
use the @ThreadSafe annotations from JCIP?

That does not follow.

Thread safety is what the class guarantees (or fails to) about its own state.
It certainly is possible to make a class thread safe and have it call
callback methods.

I haven't used the '@ThreadSafe' annotation, but based on my reading of /Java
Concurrency in Practice/ it should be fine to mark the class thread safe if it
is, indeed, thread safe.

To move expensive code blocks out of the critical section is good practice
regardless.

 

[markspace]

Hi,

if you want every method of a class to be synchronized, you
can use the @ThreadSafe annotation from JCIP and then use
FindBugs to find violations (at least that's the theory

However I'm wondering, when you have callbacks, those callbacks
are basically alien method calls and hence you cannot synchronize
every method?

So the following is incorrect because there's an alien method call:

public void synchronized doSomeStuff() {
...
if ( statechanged ) {
notifyObservers(); // alien method call
}
}

I have to write:

public void doSomeStuff() {
synchronized(this) {
...
}
if ( statechanged ) {
notifyObservers();
}
}

And then it means as soon as I've got callback I cannot
use the @ThreadSafe annotations from JCIP?

It depends how "notifyObservers()" works. If it's thread safe itself, I
think you're ok. In other words, if notifyObservers doesn't change your
object's state, then you don't have to worry about any internal
variables of notifyObservers() itself that might change, presumably
those are correctly synchronized.

(By change your internal variables, I mean directly. If
notifyObservers() triggers a call back to your object through a public
method, well I assume you have those correctly synchronized.)

In other words, the variables that you control are protected by a
synchronization block, and they were taken care of when the block ended.
There's no difference between this and just calling a method on an
object that is synchronized. It should work.

The only niggle I see is "statechanged" which needs to be volatile or a
local variable. Or do something like this:

public void doSomeStuff() {
synchronized(this) {
...
if( !statechanged )
return;
}
notifyObservers();
}

 

[neuneudr]

You don't need the annotation to make every method synchronized.

No indeed... The annotation is convenient because should you
later add, say, an unsynchronized method that break thread safety,
the FindBugs *should* find the error for you, which would be quite
convenient.

One of the problem that said is that FindBugs's JCIP annotations
support
is far from perfect and pretty much still a work in progress.

Do you mean callbacks that the synchronized class calls, or callbacks that the
class donates to other class instances to call?

The class is observable and notifies registered observers.

There's not enough information here to determine if the method is correct, or
even what "correct" means for this class.

But can't it be said, regardless of what 'correct' would mean, that
because there's an alien method call out of our control that construct
is definitely incorrect and hence cannot be correct?

Anyway, the notifyObservers() suggests that observers have been able
to register previously and are now notified.

That does not follow.

OK... My idea was that that the @ThreadSafe annotations was simply
making sure that all the references and primitives were private and
that any method modifying the state was 'synchronized'.

However I'm probably mistaken on that.

I guess having FindBugs really be able to find thread safety
violations
is a hard thing to do (it may not even be possible!?) which may
explain
why the @ThreadSafe annotations from JCIP isn't really supported yet.

 

[Arved Sandstrom]

But can't it be said, regardless of what 'correct' would mean, that
because there's an alien method call out of our control that construct
is definitely incorrect and hence cannot be correct?

Anyway, the notifyObservers() suggests that observers have been able
to register previously and are now notified.

[ SNIP]

In Effective Java, Joshua Bloch does say that "...never cede control to
the client within a synchronized method or block." He also explains that
"ceding control" translates to invoking a public or protected method that
is designed to be overridden, this being an "alien" method.

_You_ called notifyObservers() "alien". Keeping in mind why you would
classify it as such (*not* because it's a client method, *but because*
it's a client method that you cannot know the implementation details of),
then by definition it's not thread-safe, and neither is our method that
invoked it.

AHS

 

[BrianGoetz]

if you want every method of a class to be synchronized, you

can use the @ThreadSafe annotation from JCIP and then use
FindBugs to find violations (at least that's the theory

Um, no.

@ThreadSafe is simply a statement of design intent. It is up to you
to make your class thread-safe.

Simply synchronizing every method does not necessarily make a class
thread-safe, and FindBugs doesn't make this assumption either.