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!");
}
}
I am asking if I am using the synchronized block correctly
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)
Assuming the current object in _latestValidResult is FirstResult
In the doInBackground():
synchronized (_latestValidResult) {
_latestValidResult = result; //we're setting SecondResult
//still in synchronized block
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 specific final object as a lock:
private QueryResult _latestValidResult;
private final Object _latestValidResultLock = new Object();
//...
synchronized (_latestValidResultLock ) {
_latestValidResult = result;
}
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();