Speaking of thread safety?
in [Java Programming]
|
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 |