Pausing a SwingWorker with a Timer

Question

I made a SwingWorker which is fetching tweets using the twitter4j API.

The twitter API will allow 180 queries every 15 minutes, and then throw an exception with code 88 which says that the rate limit was reached. My worker is initiated from a toggle button, if the button is selected and no other worker is running (not cancelled) then a new worker starts, if it gets deselected then the worker is cancelled:

if (jStart.isSelected()) {
 if (_producer == null || _producer.isDone()) {
 _producer = new ProduceStatusWorker(new Query("a"), _x++);
 _producer.execute();
 }
} else if (_producer != null && !_producer.isDone()) {
 _producer.cancel(true);
}

Explanation of the SwingWorker:

First off this class must have exactly one worker running at all times (thus the check in the if above). A worker may stay in the pausing while loop but he must exit immediately after the while loop is finished (through if(isCancelled()))

The worker has the ability to be paused for a specific amount of time in case the API limit is reached. The worker is paused through a Timer object which notifies all the ProduceStatusWorker threads when it is finished (I have a problem here, it notifies only the ones that have created a Timer object).

The code bellow throws a fake exception to simulate making an API call that fails because of rate limiting.

If a rate limiting exception occurs and there is no countdown already running, then a Timer object will be created and started. If there is a latest QueryResult then that caries the appropriate seconds-until-reset so the Timer will finish after that amount. If the latest query result is null then a fixed timer is created (this happens in the case API calls are made, then the limit is reached, the timer is created and started, then the user stops the worker and tries to start another one before the reset is over, in that case it will not be possible to know the appropriate amount).

This is pretty much it, I just want to emphasize that _countdown is superclass global so if the user stops the worker and tries to start a new one then the same countdown wil be used.

ProduceStatusWorker with debug:

public class ProduceStatusWorker extends SwingWorker<Void, Integer> {
 private final Query _query;
 private static final int QUERY_MAX_COUNT = 100;
 private QueryResult _latestResult;
 public ProduceStatusWorker(Query query, int id) {
 _query = query;
 _query.setLang("en");
 _query.setResultType(ResultType.recent);
 _query.setCount(QUERY_MAX_COUNT);
 }
 @Override
 protected Void doInBackground() {
 QueryResult result = null;
 Set<Status> allStatuses = new HashSet<>();
 Set<Status> newStatuses = new HashSet<>();
 long lastID = Long.MAX_VALUE;
 do {
 try {
 System.out.println(_id + "." + "While-check"); // For debug puproses
 // While there is a timer running, wait (no matter which thread since "_countdown" is a global superclass variable)
 while (_countdown != null && _countdown.isRunning()) {
 System.out.println(_id + "." + j++ + ". Paused!"); // For debug puproses
 try {
 synchronized (this) {
 wait(1000);
 }
 } catch (InterruptedException ex) {
 System.out.println(_id + "." + "Background interrupted"); // For debug puproses
 }
 }
 System.out.println(_id + "." + "If-check"); // For debug puproses
 // If worker is cancelled, exit
 if (isCancelled()) {
 System.out.println(_id + "." + "Cancelled!"); // For debug puproses
 return null;
 }
 // Simulate having a API Limit exception
 if (true) {
 System.out.println(_id + "." + "Sleeping for 10 secs!"); // For debug puproses
 Thread.sleep(10000);
 throw new TwitterException("TestException");
 }
 System.out.println(_id + "." + "Trying to fetch..."); // For debug puproses
 result = DbTools.TWITTER_FACTORY.search(_query); // This is where the exception would occur normally
 _latestResult = result;
 newStatuses.clear();
 newStatuses.addAll(result.getTweets());
 System.out.println(_id + "." + i++ + ". Gathered " + newStatuses.size() + " tweets"); // For debug puproses
 allStatuses.addAll(newStatuses);
 for (Status t : newStatuses) {
 if (t.getId() < lastID) {
 lastID = t.getId();
 }
 }
 publish(allStatuses.size());
 _query.setMaxId(lastID - 1);
 } catch (TwitterException te) {
 System.out.println(_id + "." + "Fetch failed!"); // For debug puproses
 //This is the proper exception code in case of a timeout -> if (te.getErrorCode() == 88) {
 if (te.getMessage().equals("TestException")) { // Fake exception For debug puproses\
 // If a timer is already running then don't start another one
 if (_countdown == null || !_countdown.isRunning()) {
 final int resetTime;
 // If a latest result is available, then set the reset time according to it,
 // otherwise set it to a fixed amount
 if (_latestResult != null) {
 resetTime = _latestResult.getRateLimitStatus().getSecondsUntilReset();
 } else {
 resetTime = 10;
 }
 _countdown = new Timer(1000, new ActionListener() {
 private int _count = resetTime;
 @Override
 public void actionPerformed(ActionEvent e) {
 // If timer is finished, notify all ProduceStatusWorker to resume and stop the timer
 // (not working properly, notifies all except the last one because it did not create a timer object) 
 // Else set the label to the current seconds
 if (_count == 0) {
 synchronized (ProduceStatusWorker.this) {
 ProduceStatusWorker.this.notifyAll();
 }
 jTimerLabel.setText(Integer.toString(_count));
 ((Timer) e.getSource()).stop();
 } else {
 jTimerLabel.setText(Integer.toString(_count--));
 }
 }
 });
 System.out.println(_id + "." + "Starting timer: " + resetTime); // For debug puproses
 _countdown.start();
 }
 // Other type of errors here (does not matter for this current post on codereview
 } else {
 System.out.println(_id + "." + "Cancelling"); // For debug puproses
 cancel(true);
 Printer.showError(te);
 }
 } catch (InterruptedException ex) {
 Logger.getLogger(MainGUI.class.getName()).log(Level.SEVERE, null, ex);
 }
 System.out.println(_id + "." + "Do-while check"); // For debug puproses
 } while ((_countdown != null && _countdown.isRunning()) || newStatuses.size() > 0);
 return null;
 }
 // Updates the label with the current tweets held in memory
 @Override
 protected void process(List<Integer> chunks) {
 jTweetsInMemory.setText(Integer.toString(chunks.get(chunks.size() - 1)));
 }
 // Sets the button to not selected and displays a message
 @Override
 protected void done() {
 jStart.setSelected(false);
 JOptionPane.showMessageDialog(null, "Done!");
 }
}

In an older version I had created (that I preferred) I had a wait() and an if instead of the while but it could not be used since I could not notify all the ProduceStatusWorker threads.

if (_countdown != null && _countdown.isRunning()) {
 try {
 synchronized (this) {
 wait();
 }
 } catch (InterruptedException ex) {
 }
}

What I want is to know if I can use my older version and/or if there is a better way to do this all together.

• Wim Deblauwe: Maybe you can use RateLimiter of the Guava library: docs.guava-libraries.googlecode.com/git/javadoc/com/google/…

Answer 1

There's debug code strewn throughout your code.

This is bad. Clean it up: remove it.

Also... TestException? That... scares me. It needs to go. If you need to debug things, that's fine, but remove your debug code before posting on Code Review.

---

Comments should describe why your code needs to do what it does. Comments that describe how your code works should only be used in complex code segments (and even then people argue you should just clean up the code instead).

So a comment like

// If a timer is already running then don't start another one
if (_countdown == null || !_countdown.isRunning()) {

Should be removed or improved.

Right now it's confusing the readers of the code. "if a timer is already running" translates to (_countdown == null || !_countdown.isRunning())? Uh, yeah, maybe...

Comments that tell me how something works make me verify whether the code works that way. And this if statement is hard to understand.

// If a timer is already running then don't start another one

Seems simple enough.

if (_countdown == null || !_countdown.isRunning()) {

uh...

if (there is no timer OR not timer is running) {

Okay...

// If a timer is already running then don't start another one
if (there is no timer OR not timer is running) {

This is confusing, why can't it say

// If a timer is already running then don't start another one
if (there is a timer AND timer is running) {
 don't start another one
}

That'd be easy to understand. But code can't NOT do something (confusing sentence). Maybe the comment should get reversed.

// Start a new timer if there is no timer running
if (there is no timer OR not timer is running) {

Yeah, that reads better.

Code version:

// Start a new timer if there is no timer running
if (_countdown == null || !_countDown.isRunning()) {

Be really careful with comments that explain how the code works. If you get them wrong or correct but in a confusing manner, they will detract from the quality of the code.

For a bonus, wrap it in a function:

// Start a new timer if there is no timer running
if (!isTimerRunning()) {

Wrap some more things in functions and you get

// Start a new timer if there is no timer running
if (!isTimerRunning()) {
 startTimer();
}

And then the comment can go.

• K.H.: Love the readability and comment removal analysis.