Re: Speaking of thread safety?

Knute Johnson wrote:


I think the biggest issue is that the Observable class itself isn't
introducing any threading, so it doesn't really have any justification
for trying to be thread-safe nor is there any justification for client
code that is already thread-safe on its own being worried about whether
Observable is thread-safe.

There are some other points with respect to your SSCCE. I'll quote the
relevant pieces of code below as I go???


The above is a good example of what I was talking about before, with
respect to the fact that if your observer is the one introducing
threading, it will have to deal with synchronization. Why should
Observable be expected to operate correctly in this context, when it's
not the one causing the threading?

You've basically got a class Panel1 that is observable, but is
specifically going to notify observers on some other thread. Well, that
means that the observers need to be prepared to be called on some other
thread and synchronize appropriately. The Observable class should not
be expected to address this at all, nor does it need to.


And here, we see something else I was talking about before. That is, if
the observer _does_ deal with synchronization, then your object will be
fine.

In particular, repaint() is thread-safe, and so necessarily introduces
synchronization that ensures that the "value" variable is up-to-date
when inspected _by the code responding the to repaint() call_.

Now, that's an important point. In lots of examples, the interaction
between the thread happens _only_ because of the signaling done. But
paintComponent() has other ways that it could get forced to be called,
so you have an issue with atomicity that also needs to be addressed.

The code you posted does deal with that, but note that you don't
actually need to synchronize explicitly, because you can write to
"value" atomically and because repaint() implicitly synchronizes for
you. You only have to make one little change in the paintComponent()
method. Here are the new versions, without the "synchronized" statement:

     public void update(ThreadSafeObservable o, Object arg) {
         // store value here (no explicit synchronize
         // necessary, not even "volatile")
         value = ((Integer)arg).intValue();
         repaint();
         System.out.println(
          "update() called from: " + Thread.currentThread().getName());
     }

     public void paintComponent(Graphics g) {
         g.setColor(Color.WHITE);
         g.fillRect(0,0,getWidth(),getHeight());
         g.setColor(Color.BLUE);

         // atomic copy of an int
         int valueLocal = value;

         g.fillRect(getWidth() / 2 - 10, getHeight() - valueLocal, 10,
valueLocal);
         System.out.println(
          "paintComponent() called from: " +
Thread.currentThread().getName());
     }

You do need to copy "value" locally, because you don't want to get one
value in one part of setting up the fillRect() call's arguments, and a
different value in another part.

Even that is a bit iffy as to whether you really care. In particular,
any such "tearing" of values is going to occur just prior to a full
repaint in which you know the value will have a correct value. But,
it's probably good form to avoid weird drawing artifacts like that, even
if they are only transient.

Now, as for your Observable class:


In the above, synchronizing on the observers, or even copying them to a
local array, is pointless. At worst, you'll deadlock with some other
code that was also synchronizing on the observer but which is stuck
waiting for a notifyObservers() call it's trying to make to complete.
At best, you simply wind up taking a second lock that no other thread
could possibly attempt to take, because they can't get past the first
lock ("synchronized(this)").

Now, let's say you modify the above so that the notification occurs
outside the "synchronized(this)", as is the case in the actual
java.util.Observable implementation (and thus justifying them being
copied to an array). Well, you are still locking on a public reference
(i.e. known by other code), which is generally just not a good idea, and
you are locking while calling the update() method. This breaks two
important rules for multi-threaded programming:

   ??? use a private object for locking
   ??? don't hold locks while calling external code

On top of all that, the code here doesn't actually do anything for
thread-safety. At best, all you've done is ensure that your observers
will only ever have their update() method called in one thread at a
time. But that says nothing of the thread-safety of any object being
used as the observable state.

If you can be sure that the state object is _only_ ever used in the
thread where the update() method is called, then that's fine. But
otherwise, the update() method still needs to do some synchronization,
because it's still handing the object off to some other thread, in some way.

And if you can make the guarantee that the object doesn't leave the
thread where the update() method is called, then you don't actually need
to ensure that the update() method is only ever called on one thread at
a time, because the calls are mutually independent of each other.

In other words, this isn't a useful kind of synchronization to have.

I think the bottom line here is that, for your particular example, the
question of thread safety as it pertains to the java.util.Observable
class is of little concern. Since its implementation is thread-safe,
you can access that class itself safely without your own
synchronization. But it's not _introducing_ threading issues to your
own code.

You said in another post that you do in fact use multiple threads in
your own code. But in that case, the use of Observable doesn't really
change anything. You already had to do synchronization to data between
threads before, and you still do. What that synchronization needs to
be, specifically, depends on the exact implementation. It could be as
simple as above, where you rely on atomicity of variable assignments,
and the implicit synchronization of the repaint() method. Or you might
need a full lock somewhere, if you're doing things more complicated than
that.

It's still very difficult to say for sure, because the SSCCE you posted
seems mostly about an alternate "observable" implementation that isn't
quite right, and attempts to provide guarantees that it's not actually
providing, while at the same time is contrived enough that it's not
really possible to see how your _real_ program is using threading.

If you _did_ have an Observable implementation (subclass) that did in
fact impose its own thread on the design (e.g. it has a worker thread
that's used to dispatch all the notifications), then any synchronization
issues that are raised for the _Observers_ in that scenario would need
to be dealt with by the Observers themselves. The Observable subclass
wouldn't have sufficient knowledge of the synchronization needed in
order to meet the needs of the Observers in any useful, practical way.

I hope that all of the above makes sense! :)

Pete