Prev: Exception Handling
Next: First class developer: who ?
From: Peter Duniho on 12 Mar 2010 02:16
I haven't had a chance to look at the SSCCE you posted, but even without
spending time with that, I think there are useful things to say in reply
to this message below, so here's that for now. I'll look at the code
example shortly…

Knute Johnson wrote:
> [...]
> There are two issues that I was trying to solve using
> Observable/Observer. First the docs for Observable say that the thread
> calling Observer.update() may use a thread different from the one
> calling Observable.notifyObservers().

I disagree. This quote:

> "...but subclasses may change
> this order, use no guaranteed order, deliver notifications on separate
> threads..."

…specifically is discussing *subclasses*. You would know if you
introduced a subclass of Observer into your program that has a
different, thread-dangerous implementation. And yes, of course you
would have to deal with that if you did. But you don't have that
concern now, and it's perfectly reasonable to write your code with the
assumption of the behavior of the Observable class you are actually using.

> So while Observable as it is currently written will deliver
> notifications on the same thread as it was called from, that may or may
> not be true later.

For the java.util.Observable class, that will always be true. There's
nothing in the documentation that I think should be construed as a
warning to the contrary. They are _only_ talking about subclasses,
which of course can modify any or all of the behavior.

> If it isn't, the data may or may not be accessible
> from another thread. Two, how to pass a message that was to be used in
> multiple threads at the destination.

That all really depends on how the threading and passing happens. But,
if you've got a subclass of Observable that is itself introducing
threading, then it necessarily must be thread-safe internally.

As long as the thread originating the data does not mutate it after
handing it off to notifyObservers(), the object delivered to another
thread in the update() method is ensured to be coherent. The
synchronization that such a hypothetical Observable sub-class would
necessarily have to do in order to have the notification cross from one
thread to another would also take care of the coherency issues with your
object.

If the threading happens elsewhere, not as a consequence of the
Observable implementation, then the answer depends all on how that
object finds itself moving across from one thread's data to another
thread's data. But whatever the answer there, it will necessarily be
independent of the Observable. The transition from one thread to
another will happen either before the notifyObservers() method is
called, or after the Observable instance calls some Observer's update()
method.

> In my actual application that got me started down this path, I stored
> the int in a volatile and everything is solved. The Observable could
> use any thread, and the Observer could as well and my simple message is
> fine. If I could be guaranteed that Observable would always call
> notifyObservers() from the EDT and that I only wanted to use the data
> from the EDT then there is no problem again. In my application I needed
> to use the message data on the EDT and on another thread so I required
> some sort of synchronization to guarantee that it would work.

But if you're using java.util.Observable, then you don't need to worry
at all about that class causing problems. What you _do_ need is a
solution to coordinate between the code running on the EDT and the other
thread _your_ code created, but you would need that with or without the
Observable instance.

> [...]
> So I still have one major question, how could one extend Observable and
> modify notifyObservers() without access to the list of Observers? For
> the example I had to re-create both Observable and Observer. Why would
> Sun not have included a method to get the list of Observers?

Microsoft had a huge advantage in writing .NET, in that they let Sun
make a lot of the mistakes in the design and implementation of a
"managed code" environment. But the other side of that coin is that Sun
did make mistakes, and these are especially visible in the v1 stuff from
Java. As I mentioned earlier, I think that the Observable class really
is just not how Sun would do it today, if they had to do it all over again.

I mean, look at it. First, it's a class, not an interface. That's a
huge problem right there. Lots of kinds of types might want to be
observable; most need to inherit something completely different from
java.util.Observable! So you're stuck using composition (though I
suppose some might say that's a good thing, I would definitely prefer
the interface route).

Second, it uses Vector, which is one of the examples of how early Java
classes were intended to be thread-safe, and how that really didn't work
out all that well. Likewise, java.util.Observable is written to be
thread-safe, but that doesn't really mean much IMHO.

To the point you're asking though, why not include a method to get the
list of Observers, I think this again is just an oversight. For that
matter, why should subclasses be required to use the Observable class's
choice of implementation? What if you don't _want_ to use the Vector
class? Again, an argument in favor of an interface IMHO.

The fact is, if you were going to try to extend Observable, you'd
probably just override everything, treating the class as an interface
with some extra crud you just wind up carrying around that your own
implementation doesn't even use.

Pete
From: Peter Duniho on 12 Mar 2010 03:34
Knute Johnson wrote:
> [...]
> My SSCCE source code is below but I have put all of the files, including
> Sun's source for the Observable and Observer on my web site here;
>
> http://rabbitbrush.frazmtn.com/observable/
>
> I would appreciate any comments or any possible risks that you see with
> this approach.

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…

> [...]
> public class Panel1 extends JPanel {
> private final ThreadSafeObservable observable;
> private final JSpinner spin;
>
> public Panel1() {
> [...]
> public void stateChanged(ChangeEvent ce) {
> JSpinner s = (JSpinner)ce.getSource();
> final int value = ((Integer)s.getValue()).intValue();
> // start a different thread just to be sure
> new Thread(new Runnable() {
> public void run() {
> observable.setChanged();
> observable.notifyObservers(value);
> }
> }).start();
> }

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.

> [...]
> public class Panel2 extends JPanel implements ThreadSafeObserver {
> private int value;
>
> [...]
> public void update(ThreadSafeObservable o, Object arg) {
> // store value here synchronized on this the observer
> synchronized (this) {
> value = ((Integer)arg).intValue();
> }
> repaint();
> System.out.println(
> "update() called from: " + Thread.currentThread().getName());
> }

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:

> [...]
> public class ThreadSafeObservable {
> [...]
> public void notifyObservers(Object arg) {
> Object[] arrLocal;
>
> synchronized (this) {
> if (!changed)
> return;
> arrLocal = obs.toArray();
> clearChanged();
>
> for (int i=arrLocal.length-1; i>=0; i--)
> // synchronize on the observer
> synchronized (arrLocal[i]) {
> ((ThreadSafeObserver)arrLocal[i]).update(this,arg);
> }
> }
> }

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
From: Peter Duniho on 12 Mar 2010 05:21
Knute Johnson wrote:
> [...]
> If you are interested in the actual application, I have it running at:
>
> http://rabbitbrush.frazmtn.com/aviation
>
> Scroll to the bottom of the page and look at the VOR Trainer applet. The
> source code is there as well.

I was curious, so I took a look. Boy, was _that_ not conducive to me
getting to bed on time. :)

Anyway, I did note a number of problems with the implementation. Most
significantly, overuse of "volatile" in some places, and insufficient
synchronization elsewhere.

I note that in this particular program, problems with data coherency are
unlikely to cause serious issues. In the one threading scenario, you're
only modifying the data every 50ms or so, much slower than the graphical
update can occur, and minor discrepancies are going to be almost
impossible for the user to notice.

But it's good to get in the habit of implementing things properly, so
that when it _does_ really matter, you get it right. :)

The code for that particular program is somewhat larger than a typical
SSCCE, and of course who knows if you really wanted that program (or
even my version of it) saved here for posterity. :) So, I'm emailing
..java files to you, with my proposed changes to improve the quality of
the multi-threading handling.

Feel free to repost any or all of the code I email you, for whatever
purpose (second opinion from others, further discussion, etc.). For
sure, I'm not an expert on multi-threading so, while I'm sure my changes
have improved the code to some degree, I may well have missed something,
or "improved" things in the wrong way. :)

But I think it's right, and have reasonable confidence of that.

Pete
From: Knute Johnson on 13 Mar 2010 19:48
On 3/12/2010 12:34 AM, Peter Duniho wrote:
>
> 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

Pete:

Thanks very much for taking so much time to look at all of my code.
There were a lot of very good points in the code you sent me and I will
incorporate them. The one point that I'm not sure about is the call to
repaint() synchronizing data with the EDT. Looking at the code I don't
see how calling repaint() could create any synchronization. All that
happens is that some code is run on the EDT. No threads are started or
joined. I couldn't find anything in the docs that implied this either.

Thanks very much,

--

Knute Johnson
email s/nospam/knute2010/

From: Peter Duniho on 13 Mar 2010 20:18
Knute Johnson wrote:
> [...] The one point that I'm not sure about is the call to
> repaint() synchronizing data with the EDT. Looking at the code I don't
> see how calling repaint() could create any synchronization. All that
> happens is that some code is run on the EDT. No threads are started or
> joined. I couldn't find anything in the docs that implied this either.

It is a consequence of how repaint() works. There are two possibilities:

– You call repaint() in the EDT
– You call repaint() in a thread other than the EDT

In the first case, it should be obvious that whatever code executes in
that same thread before you call repaint() must produce observable
results in the same thread in the paintComponent() call that occurs as a
result of the repaint() method.

So, it's the second case that's interesting.

The basic thing to understand is that all thread synchronization relies
on memory barriers (that is, a sort of "checkpoint" that ensures that
reads and writes cannot be moved across the barrier,
execution-order-wise and time-wise; any read or write that is, in
program order, on one side of the barrier or the other must remain on
that side of the barrier in execution order).

Without memory barriers, it's impossible to synchronize threads. But,
memory barriers don't just force the specific data being affected to be
mutually observable between threads by some specific synchronization
construct (like "volatile"). They affect _all_ data used by the
cooperating threads.

Think about the "volatile" keyword, which is one of the weakest
synchronization constructs available. Even that construct provides a
guarantee that _all_ data written by a given thread before (in program
order) that thread writes to the volatile variable will be observed by
any thread reading that data _after_ (again, in program order) reading
from the same volatile variable, if the read of the volatile variable
occurs after (in execution order) the given write.

Put another way, if we have variables "int i" and "volatile int j", then
if we have threads executing like this:

thread 1 thread 2
–––––––– ––––––––
i = 0;
j = 0;
while (j != 10) { }
i = 5;
j = 10;
System.out.println(i);

The output is guaranteed to be "5". That's because "volatile"
guaranteed not only that thread 2 would see the modification to "j", but
also that, since the write to "i" happened before "j" was written, and
the read from "i" happened _after_ the write to "j" was observed, the
new value of "i" is also observed.

Now, think about what repaint() has to do when called from a thread
other than the EDT. It necessarily has to cause a call to the
paintComponent() method to happen; that's its job.

In doing so, it has to synchronize between the two threads such that the
EDT sees a change in state written by the other thread as a result of
the call to repaint(). This synchronization, whether caused by a
volatile variable, or a notify(), or whatever, will necessarily involve
a memory barrier.

This means that any change to variable state that occurs in the other
thread before the call to repaint() _must_ be observed by any code
executing in the EDT after the synchronization of the data involved in
the call to repaint().

And since the paintComponent() call that happens as a result of that
call to repaint() obviously can't happen until after that
synchronization takes place, it means that any code in the
paintComponent() method _must_ be able to see any change in state caused
by the other thread that called repaint(), because that code _also_
necessarily executes after the synchronization takes place.

Note that the guarantee is only with respect to the two threads that are
synchronized with each other. I'm not an expert in this area, and I
don't know whether any _other_ threads are also guaranteed to be able to
observe state change occurring in the EDT and the thread calling
repaint(), unless they too also participate in the synchronization (e.g.
include their own memory barrier). I suspect not, but I don't know for
sure.

But fortunately, that part doesn't matter. The only thing you really
need to worry about are the threads that do synchronize with each other,
and it happens that specifically because of the nature of that
synchronization, you can be assured that writes happening in one thread
before the synchronization point will be observed by reads happening in
a different thread after that same synchronization point. It's a direct
consequence of the whole reason for having the synchronization point
(i.e. that's specifically what the synchronization point does).

Hope that helps!

Pete
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5 6 7
Prev: Exception Handling
Next: First class developer: who ?