4

What I am trying to do ?

At the click of the Start JButton, the SwingWorker will execute. Inside the doInBackground() method, I am passing each index of arrNames, to the publish() method, so that it can be displayed inside the JTextArea.

What happened ?

If I do not keep line System.out.format("Counter : %d%n", counter); as a comment, in my doInBackground() method of the SwingWorker, then the SwingWorker is working as expected. Though if I comment it out, then the SwingWorker stops responding.

Am I doing something wrong ?


Java Version :

java version "1.7.0_25"
Java(TM) SE Runtime Environment (build 1.7.0_25-b16)
Java HotSpot(TM) Client VM (build 23.25-b01, mixed mode, sharing)

Here is the code I am using :

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

public class SwingWorkerExample1
{
    private JLabel statusLabel;
    private JTextArea tArea;
    private JButton startButton;
    private JButton stopButton;

    private BackgroundTask backgroundTask;

    private ActionListener buttonActions =
                            new ActionListener()
    {
        @Override
        public void actionPerformed(ActionEvent ae)
        {
            JButton source = (JButton) ae.getSource();
            if (source == startButton)
            {
                startButton.setEnabled(false);
                stopButton.setEnabled(true);
                backgroundTask = new BackgroundTask();
                backgroundTask.execute();
            }
            else if (source == stopButton)
            {
                backgroundTask.cancel(true);
                stopButton.setEnabled(false);
                startButton.setEnabled(true);
            }
        }
    };

    private void displayGUI()
    {
        JFrame frame = new JFrame("Swing Worker Example");
        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

        JPanel contentPane = new JPanel();
        contentPane.setBorder(
            BorderFactory.createEmptyBorder(5, 5, 5, 5));
        contentPane.setLayout(new BorderLayout(5, 5));

        statusLabel = new JLabel("Status Bar", JLabel.CENTER);

        tArea = new JTextArea(20, 20);
        tArea.setWrapStyleWord(true);
        tArea.setLineWrap(true);        
        JScrollPane textScroller = new JScrollPane();
        textScroller.setBorder(
            BorderFactory.createTitledBorder("Textual OUTPUT : "));
        textScroller.setViewportView(tArea);

        startButton = new JButton("Start");
        startButton.addActionListener(buttonActions);
        stopButton = new JButton("Stop");
        stopButton.setEnabled(false);
        stopButton.addActionListener(buttonActions);
        JPanel buttonPanel = new JPanel();
        buttonPanel.add(startButton);
        buttonPanel.add(stopButton);

        contentPane.add(statusLabel, BorderLayout.PAGE_START);
        contentPane.add(textScroller, BorderLayout.CENTER);
        contentPane.add(buttonPanel, BorderLayout.PAGE_END);

        frame.setContentPane(contentPane);
        frame.pack();
        frame.setLocationByPlatform(true);
        frame.setVisible(true);
    }

    private class BackgroundTask extends SwingWorker<Void, String>
    {
        private int counter = 0;

        private String[] arrNames = { "US Rates Strategy Cash",
            "Pavan Wadhwa(1-212) 844-4597", "Srini Ramaswamy(1-212) 844-4983",
            "Meera Chandan(1-212) 855-4555", "Kimberly Harano(1-212) 823-4996",
            "Feng Deng(1-212) 855-2555", "US Rates Strategy Derivatives",
            "Srini Ramaswamy(1-212) 811-4999",
            "Alberto Iglesias(1-212) 898-5442",
            "Praveen Korapaty(1-212) 812-3444", "Feng Deng(1-212) 812-2456",
            "US Rates Strategy Derivatives", "Srini Ramaswamy(1-212) 822-4999",
            "Alberto Iglesias(1-212) 822-5098",
            "Praveen Korapaty(1-212) 812-3655", "Feng Deng(1-212) 899-2222" };

        public BackgroundTask()
        {
            statusLabel.setText((this.getState()).toString());
            System.out.println(this.getState());
        }

        @Override
        protected Void doInBackground()
        {
            statusLabel.setText((this.getState()).toString());
            System.out.println(this.getState());
            while (!isCancelled())
            {
                counter %= arrNames.length;
                //System.out.format("Counter : %d%n", counter);
                publish(arrNames[counter]);
                counter++;
            }
            statusLabel.setText((this.getState()).toString());
            System.out.println(this.getState());
            return null;
        }

        @Override
        protected void process(java.util.List<String> messages)
        {
            for (String message : messages)
                tArea.append(String.format(message + "%n"));
        }
    }

    public static void main(String[] args)
    {
        Runnable runnable = new Runnable()
        {
            @Override
            public void run()
            {
                new SwingWorkerExample1().displayGUI();
            }
        };
        EventQueue.invokeLater(runnable);
    }
}

EDIT 1 :

As suggested, if I add Thread.sleep(...), it does work, though, it throws a InterruptedException as shown below. So the trick works. But is this the legitimate way of performing it ?

C:\Mine\JAVA\J2SE\classes>java SwingWorkerExample1
PENDING
STARTED
java.lang.InterruptedException: sleep interrupted
        at java.lang.Thread.sleep(Native Method)
        at SwingWorkerExample1$BackgroundTask.doInBackground(SwingWorkerExample1.java:108)
        at SwingWorkerExample1$BackgroundTask.doInBackground(SwingWorkerExample1.java:76)
        at javax.swing.SwingWorker$1.call(SwingWorker.java:296)
        at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
        at java.util.concurrent.FutureTask.run(FutureTask.java:166)
        at javax.swing.SwingWorker.run(SwingWorker.java:335)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
        at java.lang.Thread.run(Thread.java:724)
DONE

EDIT 2 :

Only doInBackground() is changed, that raised the above exception :

@Override
protected Void doInBackground()
{
    Runnable runnable = new Runnable()
    {
        @Override
        public void run()
        {
            statusLabel.setText((BackgroundTask.this.getState()).toString());
        }
    };
    EventQueue.invokeLater(runnable);

    System.out.println(this.getState());
    while (!isCancelled())
    {
        counter %= arrNames.length;             
        //System.out.format("Counter : %d%n", counter);
        publish(arrNames[counter]);
        try
        {Thread.sleep(30);}
        catch(InterruptedException ie)
        {ie.printStackTrace();}
        counter++;
    }
    runnable = new Runnable()
    {
        @Override
        public void run()
        {
            statusLabel.setText((BackgroundTask.this.getState()).toString());
        }
    };
    EventQueue.invokeLater(runnable);
    System.out.println(this.getState());
    return null;
}
nIcE cOw
  • 24,468
  • 7
  • 50
  • 143
  • 2
    First of, you're setting the text of the statusLabel within the doInBackground method, which means it's being updated from outside the EDT. It should be done from within the done method – MadProgrammer Jul 20 '13 at 07:14
  • 1
    Try adding a Thread.sleep(30) after the publish call.... – MadProgrammer Jul 20 '13 at 07:15
  • 1
    no, that's a documentation bug, fixed in jdk7 at least :-) Plus your statusLabel is-a label, which was never documented as thread safe ... – kleopatra Jul 20 '13 at 07:33
  • @kleopatra : True, just realized, that `setText()` is being referred to be associated with `JLabel` in my question :-) Though, `Thread.sleep(...) `does work, but it throws an InterruptedException, which again raises concerns :( – nIcE cOw Jul 20 '13 at 07:41
  • 1
    @nIcEcOw You could 1- wrap the sleep in try-catch or 2- try using Thread.yield instead. The problem is, your loop is starving the mechanism which calls publish, preventing updates – MadProgrammer Jul 20 '13 at 07:47
  • @MadProgrammer : I just realized, walking on the track you showed me, `wait()` and `notify()` can do that trick for me, though I am just wondering how to actually make it work. Though It shows me a single line, though I have to run my `SwingWorker` again to see the other line. But this `wait()/notify()` strategy does work, still working on it, to actually find a way out :-) – nIcE cOw Jul 20 '13 at 08:18
  • 1
    In this case, I'd be tempered to avoid wait/notify if you can - IMHO – MadProgrammer Jul 20 '13 at 08:21
  • please see my comments – mKorbel Jul 20 '13 at 08:29
  • as na answer, I forgot to .... – mKorbel Jul 20 '13 at 08:34
  • well, the thread throws its own interruption (as you cancelled with cancel(true)) - what do you expect :-) BTW: best to leave out _any_ access to swing in doInbackground (even if wrapped into invokelater) - the mechanism designed for keeping track of status changes is to listen ot its property. – kleopatra Jul 20 '13 at 08:44
  • BTW2: what do you _really_ want to achieve if you think wait/notify? Typically, it's not needed - the worker fits many needs (though not all, I have been told, not being a thread expert :-) – kleopatra Jul 20 '13 at 08:49
  • @kleopatra : I am expecting it to atleast run like this [ThreadCounter](http://stackoverflow.com/a/15676695/1057230), at least on stopping, it is not raising alarms, that I should worry about... With `wait/notify`, I was hoping to implement some sort of a `getter/setter` methods, so that when the `value` is set, a thread will take it and notify the other one to put another value in its place, and vice-versa for the other thread, something in lines of this example [Sync.java](https://www.dropbox.com/s/mze59q6ylbrmlz4/Sync.java). :-) – nIcE cOw Jul 20 '13 at 09:02
  • 1
    in that example you don't _interrupt_ any thread (afaics, at least :-) while here you do - so what else should it do except throwing? cancel(false) will get rid of the exception .. – kleopatra Jul 20 '13 at 09:48
  • @kleopatra : In both the examples, `Thread.sleep(1000)`, is used. And on termination, none of them use to throw any exceptions, while here, almost the same thingy, though that exception is getting raised :( – nIcE cOw Jul 20 '13 at 10:11
  • 1
    repeating: here you call worker.cancel(**TRUE**) - the true _interrupts_ the current thread (that is the thread that runs the doInBackground) - and that's what the exception is telling tells you... – kleopatra Jul 20 '13 at 10:16
  • @nIcE cOw this code never caused side [effects from various JDKs or bugs](http://stackoverflow.com/a/6114890/714968), to try to put there Thread.sleep(int), and counte is notifiead from another methods, not from doInBackground – mKorbel Jul 20 '13 at 10:20
  • @kleopatra : Too true, putting `cancel(false)` worked. May you please post this comment as an answer, I will appreciate that :-) – nIcE cOw Jul 20 '13 at 10:23

3 Answers3

3

if I add Thread.sleep(...), it does work, though, it throws a InterruptedException

The code that apparently produces the exception (copied from OP's edit):

while (!isCancelled()) {
    counter %= arrNames.length;
    // System.out.format("Counter : %d%n", counter);
    publish(arrNames[counter]);
    try {
        Thread.sleep(30); // throws
    } catch (InterruptedException ie) {
        ie.printStackTrace();
    }
    counter++;
}

The reason, though, is the code that cancels the worker (in the actionListener):

backgroundTask.cancel(true);

which explicitly tells the worker to cancel by .. interrupting the thread. From its api doc:

mayInterruptIfRunning - true if the thread executing this task should be interrupted; otherwise, in-progress tasks are allowed to complete

As an aside: catching the exception and doing nothing (thereby effectively ignoring the interrupt) is not the best of ideas. Probably not too damaging in this case - due to checking the cancelled status. Typical worker implementations either catch and return, after doing some internal cleanup if needed, or don't handle it at all.

kleopatra
  • 51,061
  • 28
  • 99
  • 211
  • Well thankyou for that, still learning new thingies, every now and then, no doubt, certain thingies, are not bound to be implemented in the production code. But happy that you pointed that out, as always +1 for that :-) – nIcE cOw Jul 20 '13 at 12:48
3

Amplifying on the other answers, don't update the GUI from your background thread, which blocks the EDT, and don't try to avoid the problem with invokeLater(). Instead, publish() the desired result and update both statusLabel and tArea in process(), as suggested below. For testing, Thread.sleep(100) simulates a small latency, but you can use Thread.yield(), as shown here. You can also update the GUI in a PropertyChangeListener, as shown here.

import java.awt.BorderLayout;
import java.awt.EventQueue;
import java.awt.event.*;
import javax.swing.*;

public class SwingWorkerExample1 {

    private JLabel statusLabel;
    private JTextArea tArea;
    private JButton startButton;
    private JButton stopButton;
    private BackgroundTask backgroundTask;
    private ActionListener buttonActions = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent ae) {
            JButton source = (JButton) ae.getSource();
            if (source == startButton) {
                startButton.setEnabled(false);
                stopButton.setEnabled(true);
                backgroundTask = new BackgroundTask();
                backgroundTask.execute();
            } else if (source == stopButton) {
                backgroundTask.cancel(true);
                stopButton.setEnabled(false);
                startButton.setEnabled(true);
            }
        }
    };

    private void displayGUI() {
        JFrame frame = new JFrame("Swing Worker Example");
        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

        JPanel contentPane = new JPanel();
        contentPane.setBorder(
            BorderFactory.createEmptyBorder(5, 5, 5, 5));
        contentPane.setLayout(new BorderLayout(5, 5));

        statusLabel = new JLabel("Status Bar", JLabel.CENTER);

        tArea = new JTextArea(20, 20);
        tArea.setWrapStyleWord(true);
        tArea.setLineWrap(true);
        JScrollPane textScroller = new JScrollPane();
        textScroller.setBorder(
            BorderFactory.createTitledBorder("Textual OUTPUT : "));
        textScroller.setViewportView(tArea);

        startButton = new JButton("Start");
        startButton.addActionListener(buttonActions);
        stopButton = new JButton("Stop");
        stopButton.setEnabled(false);
        stopButton.addActionListener(buttonActions);
        JPanel buttonPanel = new JPanel();
        buttonPanel.add(startButton);
        buttonPanel.add(stopButton);

        contentPane.add(statusLabel, BorderLayout.PAGE_START);
        contentPane.add(textScroller, BorderLayout.CENTER);
        contentPane.add(buttonPanel, BorderLayout.PAGE_END);

        frame.setContentPane(contentPane);
        frame.pack();
        frame.setLocationByPlatform(true);
        frame.setVisible(true);
    }

    private class BackgroundTask extends SwingWorker<Void, String> {

        private int counter = 0;
        private String[] arrNames = {"US Rates Strategy Cash",
            "Pavan Wadhwa(1-212) 844-4597", "Srini Ramaswamy(1-212) 844-4983",
            "Meera Chandan(1-212) 855-4555", "Kimberly Harano(1-212) 823-4996",
            "Feng Deng(1-212) 855-2555", "US Rates Strategy Derivatives",
            "Srini Ramaswamy(1-212) 811-4999",
            "Alberto Iglesias(1-212) 898-5442",
            "Praveen Korapaty(1-212) 812-3444", "Feng Deng(1-212) 812-2456",
            "US Rates Strategy Derivatives", "Srini Ramaswamy(1-212) 822-4999",
            "Alberto Iglesias(1-212) 822-5098",
            "Praveen Korapaty(1-212) 812-3655", "Feng Deng(1-212) 899-2222"};

        public BackgroundTask() {
            statusLabel.setText((this.getState()).toString());
        }

        @Override
        protected Void doInBackground() {
            while (!isCancelled()) {
                counter %= arrNames.length;
                publish(arrNames[counter]);
                counter++;
                try {
                    Thread.sleep(100); // simulate latency
                } catch (InterruptedException ex) {
                    publish("Cancelled: " + isCancelled());
                }
            }
            return null;
        }

        @Override
        protected void process(java.util.List<String> messages) {
            statusLabel.setText((this.getState()).toString());
            for (String message : messages) {
                tArea.append(String.format(message + "%n"));
            }
        }
    }

    public static void main(String[] args) {
        Runnable runnable = new Runnable() {
            @Override
            public void run() {
                new SwingWorkerExample1().displayGUI();
            }
        };
        EventQueue.invokeLater(runnable);
    }
}
Community
  • 1
  • 1
trashgod
  • 203,806
  • 29
  • 246
  • 1,045
  • Ahha, I just understood, what everyone meant by "Do not update GUI from background thread" :-) since I was not been able to get as to where to put that thingy then, if not in `doInBackground()`(How to capture those STATES for the Worker Thread). So I guess now as you have moved one statement to the `process()` method, and to catch the `DONE` state, either I can override `done()` and put one inside it, or else I can simply put that inside the `ActionListener` associated with the `Stop Button`. I hope I am right now !!! – nIcE cOw Jul 20 '13 at 14:58
  • Now got it, that `Exception Handling` mechanism, is able to catch the `DONE STATE`, this looks interesting, that even after cancelling, one can call `publish()` and indirectly `process` :-) – nIcE cOw Jul 20 '13 at 15:07
  • Correct; anything sent via `publish()` in the background thread will be seen in `process()` running on the EDT. – trashgod Jul 20 '13 at 18:19
  • +1 for answering the core question :-) Wondering, though, about recommending Thread.yield() (which currently seems to be spreading) as the api doc explicitly mentions _not_ to use it: "It is rarely appropriate to use this method." - could you elaborate a bit why you use it here? – kleopatra Jul 21 '13 at 07:22
  • @kleopatra: Good point about the revised API: [6](http://docs.oracle.com/javase/6/docs/api/java/lang/Thread.html#yield%28%29), [7](http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#yield%28%29). In this case, `yield()` is "useful for debugging or testing purposes" to simulate the latency that is inherent in a typical real world problem. Without at least `yield()`, the background thread causes [starvation](http://docs.oracle.com/javase/tutorial/essential/concurrency/starvelive.html) and saturates the `Timer`, mentioned [here](http://stackoverflow.com/a/11838349/230513). – trashgod Jul 21 '13 at 08:49
  • my bad: re-reading your answer, I misunderstood your sentence earlier (and didn't look into the code) - sorry. Would upvote again, if possible :-) – kleopatra Jul 21 '13 at 09:54
2

comments

  • @kleopatra that's a documentation bug, fixed in jdk7 at least :-), please which of JDKs those bugs are shown as from carousel ....,

  • wait/notify is for thread, SwingWorker is Future, very bad implemented, means without notifiers, you put something to the tube and waiting on another side,

  • seems like as nothing betweens one and second side of this tube, this reason why I tried to invoke Thread, Runnable, Executor(Runnable) from doInBackground, and to ignore publish, process, setProcess

  • another funny issue is to get() and exception(s) all exception, not only 1st. from one and second side of this tube

  • there are two ways how to use SwingWokrer

    1. try to avoid to use SwingWorker

    2. use doInBackground as bridge for workers thread, for output to use publish, process, setProcess, wait for done(), and use done() as notifier for get an exception, notifier that SwingWorker ended

mKorbel
  • 109,525
  • 20
  • 134
  • 319
  • +1, I liked this second point, as already pointed by @MadProgrammer, I am still thinking as to why this approach is bad. As far as I understood till now, putting the `EDT` on `wait()` so that it can be `notified` about a certain occurence of an event is bad, in every way. Then it turns out, that why not keep that `System.out.println(...)` thingy as it is, atleast then the whole thingy is working as expected :-) – nIcE cOw Jul 20 '13 at 08:40