0

I have a SwingWorker which makes calls to the twitter API and gets some results back, everytime a result is received I first update the member variable _latestValidResult, then I take a list of tweets from the result and add it to my list and then I publish that list's size. Then in publish I need to use _latestValidResult in order to access some API timer limits so I can update my GUI.

I have a synchronized block in the doInBackground() where I update _latestValidResult and in process() where I use _latestValidResult to acquire the timer limits.

I am asking if I am using the synchronized block correctly because I am getting a warning from my IDE that I am synchronizing on a non-final variable.

This is the barebone code with some pseudo-code so it won't give you a headache:

public class ProduceStatusWorker extends SwingWorker<Void, Integer> {

    private QueryResult _latestValidResult;

    @Override
    protected Void doInBackground() {
        QueryResult result = null;
        Set<Status> allStatuses = new HashSet<>();
        do {
            try {
                if (isCancelled()) {
                    return null;
                }

                result = making_api_call_here;

                // If the call succeeded (no exception) then copy the result to the _latestValidResult
                synchronized (_latestValidResult) {
                    _latestValidResult = result;
                }

                allStatuses.addAll(result.getTweets());

                publish(allStatuses.size());

            } catch (TwitterException te) {
                // Handle exceptions here
            }

        } while (more_statuses_can_be_retrieved);

        return null;
    }

    @Override
    protected void process(List<Integer> chunks) {
        final int apiCallsTotal;
        final int apiCallsLeft;

        // Get the variables I need from _latestValidResult
        synchronized (_latestValidResult) {
            apiCallsTotal = _latestValidResult.getRateLimitStatus().getLimit();
            apiCallsLeft = _latestValidResult.getRateLimitStatus().getRemaining();
        }

        // Update GUI according to the variables
        jAPICallsLeftLabel.setText(Integer.toString(apiCallsLeft) + "/" + Integer.toString(apiCallsTotal));

        // Update GUI according to the publish
        jTweetsInMemory.setText(Integer.toString(chunks.get(chunks.size() - 1)));
    }
}

Here is the whole code of the ProduceStatusWorker class:

public class ProduceStatusWorker extends SwingWorker<Void, Integer> {

    private final Query _query;

    private QueryResult _latestValidResult;

    private static final int QUERY_MAX_COUNT = 100;

    public ProduceStatusWorker(Query query) {
        _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<>();
        long lastID = Long.MAX_VALUE;
        do {
            try {
                while (timerIsRunning(_countdown) && !isCancelled()) {
                    try {
                        synchronized (this) {
                            wait(1000);
                        }
                    } catch (InterruptedException ex) {
                    }
                }

                if (isCancelled()) {
                    return null;
                }

                result = DbTools.TWITTER_FACTORY.search(_query);

                synchronized (_latestValidResult) {
                    _latestValidResult = result;
                }

                allStatuses.addAll(result.getTweets());

                for (Status status : result.getTweets()) {
                    if (status.getId() < lastID) {
                        lastID = status.getId();
                    }
                }
                publish(allStatuses.size());
                _query.setMaxId(lastID - 1);

            } catch (TwitterException te) {
                if (te.getErrorCode() == 88) {
                    if (!timerIsRunning(_countdown)) {
                        _countdown = new Timer(1000, new ActionListener() {
                            private int _count = _latestValidResult != null
                                    ? _latestValidResult.getRateLimitStatus().getSecondsUntilReset() : 10;

                            @Override
                            public void actionPerformed(ActionEvent e) {
                                if (_count == 0) {
                                    synchronized (ProduceStatusWorker.this) {
                                        ProduceStatusWorker.this.notifyAll();
                                    }
                                    jTimeLeftLabel.setText(DurationFormatUtils.formatDuration(_count-- * 1000, "mm:ss"));
                                    ((Timer) e.getSource()).stop();
                                } else {
                                    jTimeLeftLabel.setText(DurationFormatUtils.formatDuration(_count-- * 1000, "mm:ss"));
                                }
                            }
                        });
                        _countdown.start();
                    }

                } else {
                    cancel(true);
                    Printer.showError(te);
                }
            }

        } while ((_countdown != null && _countdown.isRunning()) || result.getTweets().size() > 0);

        return null;
    }

    @Override
    protected void process(List<Integer> chunks) {
        final int apiCallsTotal;
        final int apiCallsLeft;

        synchronized (_latestValidResult) {
            apiCallsTotal = _latestValidResult.getRateLimitStatus().getLimit();
            apiCallsLeft = _latestValidResult.getRateLimitStatus().getRemaining();
        }

        jAPICallsLeftLabel.setText(Integer.toString(apiCallsLeft) + "/" + Integer.toString(apiCallsTotal));
        jTweetsInMemory.setText(Integer.toString(chunks.get(chunks.size() - 1)));
    }

    @Override
    protected void done() {
        jStart.setSelected(false);
        JOptionPane.showMessageDialog(null, "Done!");
    }
}
Aki K
  • 1,222
  • 1
  • 27
  • 49
  • The easiest way is to create a dedicated monitor object (`private static final Object monitor = new Object();`) and to use this to synchronize. That way every thread that uses this reference has to wait until the thread that "entered" the block first, finished his work there. You may also synchronize using `ProduceStatusWorker.class`. – Tom Aug 23 '14 at 11:21

4 Answers4

3

You won't get the desired effect. Locks are only useful if all participants use the same one lock. With your code, there will be many locks for one thing you are trying to control. Conversely, it doesn't matter what is being used as lock as long as it is the same one.

One straightforward way of doing this is doing synchronized (this). However, the recommended way is to do :

private final Object lock = new Object();
//
void method(){
    synchronized(lock){
        // stuff
    }
}

so that unrelated participants can't use your lock accidentally (This code admittedly looks pretty silly and IMO is a design flaw of Java).

In your case however, you could simply do:

volatile QueryResult latestValidResult;

and be done with it. This is because QueryResult is effectively immutable (from what I can guess), and because there is multiple readers but only one writer, you just need to make the latest valid result visible to the other threads. In such a case, volatile is sufficient and IMO is easier to understand what you are trying to do.

Enno Shioji
  • 26,542
  • 13
  • 70
  • 109
2

I am asking if I am using the synchronized block correctly

Simple answer: no

synchronized (_latestValidResult) {

This will not get a lock on the member, but on the object referenced by this member. (Actually, it will probably throw a NullPointerException the first time)

Step by step

Assuming the current object in _latestValidResult is FirstResult

  1. In the doInBackground():

    synchronized (_latestValidResult) {
    _latestValidResult = result; //we're setting SecondResult
    //still in synchronized block

  2. At this exact time, while we are in the synchronized block, another thread runs the process() function:

    synchronized (_latestValidResult) {

    This get a lock on SecondResult, threads run simultaneously. I don't think that's what you were expecting by using the synchronized keyword

Use a lock

Use a specific final object as a lock:

private QueryResult _latestValidResult;
private final Object _latestValidResultLock = new Object();

//...

    synchronized (_latestValidResultLock ) {
        _latestValidResult = result;
    }

Maybe you don't even need to synchronize

I haven't go deeply in your code to see if it would work, but maybe you can do something like this:

private volatile QueryResult _latestValidResult;

//...

    _latestValidResult = result;

//...

    QueryResult latestValidResult = _latestValidResult;
    apiCallsTotal = latestValidResult.getRateLimitStatus().getLimit();
    apiCallsLeft = latestValidResult.getRateLimitStatus().getRemaining();
Volune
  • 4,324
  • 22
  • 23
1

You should do something like this, so that the _latestValidResult doesnt get modified till the time its read by the process function. using a final object as lock will make sure that at a time only one thread holds the lock and is either reading or modifying it.

private QueryResult _latestValidResult;
private final Object lock = new Object();

protected void doInBackground(){

    synchronized(lock){
        _latestValidResult = result;
    }
}

protected void process(List<Integer> chunks){
    synchronized(lock){
         apiCallsTotal = _latestValidResult.getRateLimitStatus().getLimit();
         apiCallsLeft = _latestValidResult.getRateLimitStatus().getRemaining();
    }
}
Sumeet Sharma
  • 2,573
  • 1
  • 12
  • 24
0

Don't synchronize on the variable you're modifying. Instead use a Lock or a separate lock variable on which to synchronize (you can even make it static final to keep the IDE from complaining).

Kayaman
  • 72,141
  • 5
  • 83
  • 121