5
\$\begingroup\$

I am experimenting with the produce and consume concept while trying to incorporate it into my own project.

I basically have two Workers in a small example I made:

ProduceWorker: Populates a list with random numbers with a delay to emulate a real world adding in the doInBackground() method. Then in the process() that list is copied to another list which is used in the second worker and the first list is cleared.

public class ProduceWorker extends SwingWorker<Void, List<Integer>> {

        @Override
        protected Void doInBackground() throws InterruptedException {
            while (true) {
                for (int i = 0; i < 10; i++) {
                    _addingList.add(_rand.nextInt(100));
                    Thread.sleep(_rand.nextInt(300));
                }
                publish(_addingList);
            }
        }

        @Override
        protected void process(List<List<Integer>> chunks) {
            jReadyNumber.setText(Integer.toString(chunks.get(0).size()));

            synchronized (_addingList) {
                synchronized (_editingList) {
                    _editingList = new ArrayList<>(_addingList);
                    _addingList.clear();
                }
            }
        }
    }

ConsumeWorker: Grabs the second list (copy of the first)and just emulates doing something in it's elements with a random delay in the doInBackground() method. Then in the process() that list is cleared.

public class ConsumeWorker extends SwingWorker<Void, List<Integer>> {

        @Override
        protected Void doInBackground() throws InterruptedException {
            while (true) {
                for (int number : _editingList) {
                    Thread.sleep(_rand.nextInt(100));
                }
                publish(_editingList);
            }
        }

        @Override
        protected void process(List<List<Integer>> chunks) {
            jDoneNumber.setText(Integer.toString(chunks.get(0).size()));
            synchronized (_editingList) {
                _editingList.clear();
            }
        }
    }

Now I have this example in a JFrame with some labels and a toggle button that when it gets pressed, both workers start executing.

Here is the whole MainGui.java:

package gui;

import java.util.ArrayList;
import java.util.List;
import java.util.Random;
import javax.swing.SwingWorker;

public class MainGui extends javax.swing.JFrame {

    public MainGui() {
        initComponents();

        _addingList = new ArrayList<>();
        _editingList = new ArrayList<>();

        _rand = new Random();
    }

    @SuppressWarnings("unchecked")
    // <editor-fold defaultstate="collapsed" desc="Generated Code">                          
    private void initComponents() {

        jLabel1 = new javax.swing.JLabel();
        jLabel2 = new javax.swing.JLabel();
        jReadyNumber = new javax.swing.JLabel();
        jDoneNumber = new javax.swing.JLabel();
        jStartToggle = new javax.swing.JToggleButton();

        setDefaultCloseOperation(javax.swing.WindowConstants.EXIT_ON_CLOSE);

        jLabel1.setText("Ready:");

        jLabel2.setText("Done:");

        jReadyNumber.setText("0");

        jDoneNumber.setText("0");

        jStartToggle.setText("Start");
        jStartToggle.addChangeListener(new javax.swing.event.ChangeListener() {
            public void stateChanged(javax.swing.event.ChangeEvent evt) {
                jStartToggleStateChanged(evt);
            }
        });
        jStartToggle.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(java.awt.event.ActionEvent evt) {
                jStartToggleActionPerformed(evt);
            }
        });

        javax.swing.GroupLayout layout = new javax.swing.GroupLayout(getContentPane());
        getContentPane().setLayout(layout);
        layout.setHorizontalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(layout.createSequentialGroup()
                .addGap(136, 136, 136)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addComponent(jStartToggle)
                    .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.TRAILING, false)
                        .addGroup(layout.createSequentialGroup()
                            .addComponent(jLabel2)
                            .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED, javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)
                            .addComponent(jDoneNumber))
                        .addGroup(layout.createSequentialGroup()
                            .addComponent(jLabel1)
                            .addGap(18, 18, 18)
                            .addComponent(jReadyNumber))))
                .addContainerGap(199, Short.MAX_VALUE))
        );
        layout.setVerticalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(layout.createSequentialGroup()
                .addGap(89, 89, 89)
                .addComponent(jStartToggle)
                .addGap(18, 18, 18)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(jLabel1)
                    .addComponent(jReadyNumber))
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(jLabel2)
                    .addComponent(jDoneNumber))
                .addContainerGap(129, Short.MAX_VALUE))
        );

        pack();
    }// </editor-fold>                        

    private void jStartToggleStateChanged(javax.swing.event.ChangeEvent evt) {                                          
        if (jStartToggle.isSelected()) {
            jStartToggle.setText("Stop");
        } else {
            jStartToggle.setText("Start");
        }
    }                                         

    private void jStartToggleActionPerformed(java.awt.event.ActionEvent evt) {                                             
        if (jStartToggle.isSelected()) {
            new ProduceWorker().execute();
            new ConsumeWorker().execute();
        }
    }                                            

    public class ProduceWorker extends SwingWorker<Void, List<Integer>> {

        @Override
        protected Void doInBackground() throws InterruptedException {
            while (true) {
                for (int i = 0; i < 10; i++) {
                    _addingList.add(_rand.nextInt(100));
                    Thread.sleep(_rand.nextInt(300));
                }
                publish(_addingList);
            }
        }

        @Override
        protected void process(List<List<Integer>> chunks) {
            jReadyNumber.setText(Integer.toString(chunks.get(0).size()));

            synchronized (_addingList) {
                synchronized (_editingList) {
                    _editingList = new ArrayList<>(_addingList);
                    _addingList.clear();
                }
            }
        }
    }

    public class ConsumeWorker extends SwingWorker<Void, List<Integer>> {

        @Override
        protected Void doInBackground() throws InterruptedException {
            while (true) {
                for (int number : _editingList) {
                    Thread.sleep(_rand.nextInt(100));
                }
                publish(_editingList);
            }
        }

        @Override
        protected void process(List<List<Integer>> chunks) {
            jDoneNumber.setText(Integer.toString(chunks.get(0).size()));
            synchronized (_editingList) {
                _editingList.clear();
            }
        }
    }

    /**
     * @param args the command line arguments
     */
    public static void main(String args[]) {
        /* Set the Nimbus look and feel */
        //<editor-fold defaultstate="collapsed" desc=" Look and feel setting code (optional) ">
        /* If Nimbus (introduced in Java SE 6) is not available, stay with the default look and feel.
         * For details see http://download.oracle.com/javase/tutorial/uiswing/lookandfeel/plaf.html 
         */
        try {
            for (javax.swing.UIManager.LookAndFeelInfo info : javax.swing.UIManager.getInstalledLookAndFeels()) {
                if ("Nimbus".equals(info.getName())) {
                    javax.swing.UIManager.setLookAndFeel(info.getClassName());
                    break;
                }
            }
        } catch (ClassNotFoundException ex) {
            java.util.logging.Logger.getLogger(MainGui.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (InstantiationException ex) {
            java.util.logging.Logger.getLogger(MainGui.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (IllegalAccessException ex) {
            java.util.logging.Logger.getLogger(MainGui.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (javax.swing.UnsupportedLookAndFeelException ex) {
            java.util.logging.Logger.getLogger(MainGui.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        }
        //</editor-fold>

        /* Create and display the form */
        java.awt.EventQueue.invokeLater(new Runnable() {
            public void run() {
                new MainGui().setVisible(true);
            }
        });
    }

    List<Integer> _addingList;
    List<Integer> _editingList;

    Random _rand;

    // Variables declaration - do not modify                     
    private javax.swing.JLabel jDoneNumber;
    private javax.swing.JLabel jLabel1;
    private javax.swing.JLabel jLabel2;
    private javax.swing.JLabel jReadyNumber;
    private javax.swing.JToggleButton jStartToggle;
    // End of variables declaration                   
}

Is there any way to make it better code wise and performance wise?

\$\endgroup\$
5
\$\begingroup\$

Don't lock on non-final objects.

The reason for this is that something later may change the variable. I might say _addingList = new ArrayList<>(); and then the synchronized blocks fail.

    //ProduceWorker
    @Override
    protected void process(List<List<Integer>> chunks) {
        jReadyNumber.setText(Integer.toString(chunks.get(0).size()));

        synchronized (_addingList) {
            synchronized (_editingList) {
                _editingList = new ArrayList<>(_addingList);
                _addingList.clear();
            }
        }
    }

    //ConsumeWorker
    @Override
    protected void process(List<List<Integer>> chunks) {
        jDoneNumber.setText(Integer.toString(chunks.get(0).size()));
        synchronized (_editingList) {
            _editingList.clear();
        }
    }

What could happen here is the following:

  • Thread A enters ConsumeWorker.process and yields.
  • Thread B enters ProduceWorker.process and obtains a lock on _addingList and _editingList. Then it runs _editingList = new ArrayList<>(_addingList); and it yields.
  • Thread A obtains a lock on _editingList (It can, because the instance changed) and runs _editingList.clear(). It ends the function and yields.
  • Thread B runs _addingList.clear(), ends the function and yields.

You've just cleared both your lists.

I'm not sure that was the intended goal, but it could happen. Maybe it can't happen right now due to certain circumstances. But you'll make a future change and all of a sudden it will happen and you will have bugs and these bugs will be near impossible to reproduce... you'll get a gradually building list of tickets of customers who report that sometimes, it doesn't work.

Make a special final Object _addingListLock that you create once and never mess with again. Then synchronize on that. Do the same for _editingList.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.