Speaking of thread safety?
in [Java Help]
|
Prev: Exception Handling
Next: First class developer: who ?
From: markspace on 11 Mar 2010 17:20 I don't know if you had time to read my earlier reply, but after I posted it I starting thinking about the little design I posted. I'm thinking that what I said--that it is probably good design to allow a thread safe method of the observer--is just wrong. There's two competing principles in design: 1. usefulness or ease of use, and 2. pithiness. The opposite of pithiness is gold plating, a known anti-pattern. If you over emphasize 1, then you risk a baroque, ornamented design that takes 20 method calls to accomplish what should take just one method call. If you over emphasize 2, then you risk a stark design that only works for your particular need, but can't be extended and is difficult to maintain due to it being too embedded in the particular code you wrote. Always there's a balance to maintain. Anyway, what I'm trying to say here is that because you have a JPanel to begin with, you have events on the EDT anyway, so why not just skip the thread safety and say all methods have to be called on the EDT? For example: Knute Johnson wrote: > Panel1 p1 = new Panel1(); > Panel2 p2 = new Panel2(); > p1.addObserver(p2); This is EXACTLY the example I was going to provide, implemented on the EDT just as you have. And it doesn't need to be specially thread safe, it's naturally called on the EDT anyway. Yes, making the classes not thread safe risks making the class hard to use. But most Swing classes aren't thread safe, and folks are used to dealing with that. So there's a well developed body of knowledge for working with Swing classes that aren't thread safe, and I don't think you're risking much by requiring the user be responsible for thread safety. By contrast, especially after looking at your code, I think you are risking a baroque, gold-plated, hard-to-maintain design by trying to make the class totally thread safe. Events fire on the EDT because it's fast to do so; trying to spawn a separate thread just to re-issue an event to update your 2nd panel on the EDT again seems like mis-design. (What one of my professors used to call "going around your elbow to get from your forefinger to your thumb.") Anyway, here's my design. I nixed your actual panel because it was just easier to use a JTextArea for the example, but the whole thing fires on the EDT, so it doesn't matter. It's all thread safe, and I don't have to make any threads, and therefore things should happen quickly and the UI should stay fast too. The code is shorter, and runs faster. Win-win. (Note the JSpinner doesn't actually say that a ChangeListener is guaranteed to be called on the EDT. I assume that it is. Therefore any observer added can assume that it's update() method is also called on the EDT, because of my design.) Here's the main class, you can see it's pretty short and too the point: package panelstest; import javax.swing.BoxLayout; import javax.swing.JFrame; import javax.swing.JPanel; import javax.swing.SwingUtilities; public class Main { public static void main(String[] args) { SwingUtilities.invokeLater( new Runnable() { public void run() { createAndShowGui(); } } ); } private static void createAndShowGui() { JPanel panel = new JPanel(); BoxLayout layout = new BoxLayout( panel, BoxLayout.PAGE_AXIS ); panel.setLayout( layout ); SpinnerPanel spinPan = new SpinnerPanel(); VariableRectanglePanel vrp = new VariableRectanglePanel(); spinPan.addOberver( vrp ); panel.add( spinPan ); panel.add( vrp ); JFrame frame = new JFrame("Oberver Test" ); frame.add( panel ); frame.setLocationRelativeTo( null ); frame.setDefaultCloseOperation( JFrame.EXIT_ON_CLOSE ); frame.pack(); frame.setVisible( true ); } } The rest of the code is kinda baroque because I was lazy and used a GUI editor to generate the panels, so they're nicely laid out. Phear the code generator, I guess. If you suck these classes into an IDE and use code-folding to hide the methods that have the auto-generated comment, you'll see that they're very simple in concept. The code I had to add is pretty easy. First the easy bit, my observer interface: package panelstest; public interface MyObserver { void update( Object message ); } Now for the hold-your-nose bits: /* * To change this template, choose Tools | Templates * and open the template in the editor. */ /* * SpinnerPanel.java * * Created on Mar 11, 2010, 1:18:21 PM */ package panelstest; import java.util.ArrayList; import javax.swing.SwingUtilities; import javax.swing.event.ChangeEvent; import javax.swing.event.ChangeListener; /** * * @author Brenden */ public class SpinnerPanel extends javax.swing.JPanel { private ArrayList<MyObserver> observers = new ArrayList<MyObserver>(); /** Creates new form SpinnerPanel */ public SpinnerPanel() { initComponents(); jSpinner1.addChangeListener( new ChangeListener() { public void stateChanged( ChangeEvent e ) { System.err.println( "EDT: "+SwingUtilities.isEventDispatchThread() ); notifyObservers( jSpinner1.getValue() ); } } ); } /** This method is called from within the constructor to * initialize the form. * WARNING: Do NOT modify this code. The content of this method is * always regenerated by the Form Editor. */ @SuppressWarnings("unchecked") // <editor-fold defaultstate="collapsed" desc="Generated Code"> private void initComponents() { jLabel1 = new javax.swing.JLabel(); jSpinner1 = new javax.swing.JSpinner(); setBorder(javax.swing.BorderFactory.createTitledBorder("Spinner -- Observable")); jLabel1.setText("Spin me! "); javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this); this.setLayout(layout); layout.setHorizontalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addGroup(layout.createSequentialGroup() .addContainerGap() .addComponent(jLabel1) ..addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED) .addComponent(jSpinner1, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE) .addContainerGap(59, Short.MAX_VALUE)) ); layout.setVerticalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addGroup(layout.createSequentialGroup() .addContainerGap() ..addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE) .addComponent(jLabel1) .addComponent(jSpinner1, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE)) .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)) ); }// </editor-fold> // Variables declaration - do not modify private javax.swing.JLabel jLabel1; private javax.swing.JSpinner jSpinner1; // End of variables declaration /** NOT THREAD SAFE! Call only on the EDT */ public void addOberver( MyObserver observer ) { observers.add( observer ); } private void notifyObservers( Object message ) { for( MyObserver o : observers ) { o.update( message ); } } } ------------And------------- /* * To change this template, choose Tools | Templates * and open the template in the editor. */ /* * VariableRectanglePanel.java * * Created on Mar 11, 2010, 1:23:27 PM */ package panelstest; /** * * @author Brenden */ public class VariableRectanglePanel extends javax.swing.JPanel implements MyObserver { /** Creates new form VariableRectanglePanel */ public VariableRectanglePanel() { initComponents(); } /** This method is called from within the constructor to * initialize the form. * WARNING: Do NOT modify this code. The content of this method is * always regenerated by the Form Editor. */ @SuppressWarnings("unchecked") // <editor-fold defaultstate="collapsed" desc="Generated Code"> private void initComponents() { jLabel1 = new javax.swing.JLabel(); jScrollPane1 = new javax.swing.JScrollPane(); jTextArea1 = new javax.swing.JTextArea(); setBorder(javax.swing.BorderFactory.createTitledBorder("Variable Rectangle -- Observer")); jLabel1.setText("Variable Rectangle:"); jTextArea1.setColumns(20); jTextArea1.setRows(5); jScrollPane1.setViewportView(jTextArea1); javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this); this.setLayout(layout); layout.setHorizontalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addGroup(layout.createSequentialGroup() .addContainerGap() .addComponent(jLabel1) ..addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED) .addComponent(jScrollPane1, javax.swing.GroupLayout.DEFAULT_SIZE, 245, Short.MAX_VALUE) .addContainerGap()) ); layout.setVerticalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addGroup(layout.createSequentialGroup() .addContainerGap() ..addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addComponent(jScrollPane1, javax.swing.GroupLayout.DEFAULT_SIZE, 154, Short.MAX_VALUE) .addComponent(jLabel1)) .addContainerGap()) ); }// </editor-fold> // Variables declaration - do not modify private javax.swing.JLabel jLabel1; private javax.swing.JScrollPane jScrollPane1; private javax.swing.JTextArea jTextArea1; // End of variables declaration public void update( Object message ) { jTextArea1.append( "Update: "+message.toString()+"\n" ); } }
From: Knute Johnson on 11 Mar 2010 21:36 On 3/11/2010 2:20 PM, markspace wrote: > I don't know if you had time to read my earlier reply, but after I > posted it I starting thinking about the little design I posted. I'm > thinking that what I said--that it is probably good design to allow a > thread safe method of the observer--is just wrong. > > There's two competing principles in design: 1. usefulness or ease of > use, and 2. pithiness. The opposite of pithiness is gold plating, a > known anti-pattern. > > If you over emphasize 1, then you risk a baroque, ornamented design that > takes 20 method calls to accomplish what should take just one method call. > > If you over emphasize 2, then you risk a stark design that only works > for your particular need, but can't be extended and is difficult to > maintain due to it being too embedded in the particular code you wrote. > Always there's a balance to maintain. > > Anyway, what I'm trying to say here is that because you have a JPanel to > begin with, you have events on the EDT anyway, so why not just skip the > thread safety and say all methods have to be called on the EDT? > > For example: > > Knute Johnson wrote: >> Panel1 p1 = new Panel1(); >> Panel2 p2 = new Panel2(); >> p1.addObserver(p2); > > > This is EXACTLY the example I was going to provide, implemented on the > EDT just as you have. And it doesn't need to be specially thread safe, > it's naturally called on the EDT anyway. > > Yes, making the classes not thread safe risks making the class hard to > use. But most Swing classes aren't thread safe, and folks are used to > dealing with that. So there's a well developed body of knowledge for > working with Swing classes that aren't thread safe, and I don't think > you're risking much by requiring the user be responsible for thread safety. > > By contrast, especially after looking at your code, I think you are > risking a baroque, gold-plated, hard-to-maintain design by trying to > make the class totally thread safe. Events fire on the EDT because it's > fast to do so; trying to spawn a separate thread just to re-issue an > event to update your 2nd panel on the EDT again seems like mis-design. > (What one of my professors used to call "going around your elbow to get > from your forefinger to your thumb.") > > Anyway, here's my design. I nixed your actual panel because it was just > easier to use a JTextArea for the example, but the whole thing fires on > the EDT, so it doesn't matter. It's all thread safe, and I don't have to > make any threads, and therefore things should happen quickly and the UI > should stay fast too. The code is shorter, and runs faster. Win-win. > > (Note the JSpinner doesn't actually say that a ChangeListener is > guaranteed to be called on the EDT. I assume that it is. Therefore any > observer added can assume that it's update() method is also called on > the EDT, because of my design.) Thanks for the code, the important parts were not that hard to read. 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(). "...but subclasses may change this order, use no guaranteed order, deliver notifications on separate threads..." 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. 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. 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. 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. 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? -- Knute Johnson email s/nospam/knute2010/
From: John B. Matthews on 11 Mar 2010 22:25 In article <gv%ln.134$%H1.119(a)newsfe23.iad>, Knute Johnson <nospam(a)rabbitbrush.frazmtn.com> wrote: >>> <http://sites.google.com/site/drjohnbmatthews/threadwatch> [...] > >> In the application that I am writing, I am passing an Integer). > >> In the Observer.update() method I store that Integer. I'm storing > >> it in a volatile int so that solves the visibility problems for > >> the reading thread but what if you had a mutable Object to pass? > > > > I'm not sure about that. > > I'm thinking now that maybe I didn't design this quite correctly. I have to acknowledge your clear SSCCEs and appealing demos. I think writing your own Observer/Observable implementations gets around the limitations of the Java 1.0 version. The hidden synchronized block in notifyObservers() helps me understand why my demo works at all. -- John B. Matthews trashgod at gmail dot com <http://sites.google.com/site/drjohnbmatthews>
From: markspace on 11 Mar 2010 22:57 Knute Johnson wrote: > 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. As I mention, there is a large body of knowledge which folks learn to deal with situations like this. What do you do with any listener which is called on the EDT, when you want to run some portion of its code off the EDT? One popular way is to use a SwingWorker. Easy, standardized, and most people will instantly recognize the pattern and know what is going on. That's not to say what you did is wrong, just that there might be better ways of doing what you did, by re-using existing classes and paradigms. > > 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 haven't looked at this yet. If I get some time I will. > > So I still have one major question, how could one extend Observable and > modify notifyObservers() without access to the list of Observers? For Well, I think I did that in my code, minus the bit about extending the Observable. The "notify" code starts on the EDT due to the JSpinner's internals, and I just invoke each observer on the same thread. I should probably add a assert to verify this, but I didn't make any test harness so I didn't add any asserts... > 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? > Beats me. They do allow you to get a list of PropertyChangeListeners, which is the interface I ended up using (indirectly). Possibly it may not be safe to allow an external user to get at the internals of a class that wants to synchronize and protect it's private data, so there may in fact be a good reason.
From: Peter Duniho on 12 Mar 2010 01:48
Knute Johnson wrote: > The point is that you can't according to the docs, guarantee that > Observable's call to update() is done on a particular thread. I see nothing in the documentation that suggests that. Not about the _default_ implementation. It does say that about alternate implementations – that is, subclasses of Observable – but those implementations would presumably come with their own documentation. Pete |