Hylke Bron Hylke Bron - 4 months ago 22
Android Question

Use synchronized on local final variable

I'm working on some issue where I have an interface implementation (of GoogleMaps tile provider) in which the method requires its data immediately (not in a callback), but for me to get the data I have to call a method which returns the data in a callback. I have to link these 2 together, and I have something (which I think works, I have yet to test it), but I am worried about some Android Studio warnings I get.

This is the code I've written:

@Override
public Tile getTile(int x, int y, int zoom) {
// If the tile provider is not available, return null
if(tileProvider == null) {
return NO_TILE;
}

// Define the tile request
final TileRequest tileRequest = new TileRequest(tileProvider.getLayer().getTileWidth(), tileProvider.getLayer().getTileHeight());

// Make the call to get the tile data, which, depending on the situation, can possibly
// be handled by another thread.
tileProvider.getTile(x, y, zoom, new GetDataListener<byte[]>() {
@Override
public void onGetData(byte[] data, Exception exception) {
synchronized (tileRequest) {
tileRequest.data = data;
tileRequest.exceptionOccurred = exception != null;
tileRequest.finishedRequest = true;
tileRequest.notify();
}
}
});

synchronized (tileRequest) {
// If, before this statement was reached, the request has already been finished, call and return getTile immediately.
if(tileRequest.finishedRequest) {
return tileRequest.getTile();
} else {
try {
// Wait for the tileRequest to be finished
tileRequest.wait();

// Once it is finished (in the callback a notify is called as soon as its finished, so thats how this code is reached)
// the tile data is available, return the tile
return tileRequest.getTile();

} catch(InterruptedException ex) {
// Exception occurred, return null so GoogleMaps will try it again later
logger.error("Exception in getTile method: {}", ex.getLocalizedMessage());
ex.printStackTrace();
return null;
}
}
}
}


So what Android Studio is giving me, on the second
synchronized (tileRequest) {
line, is the following warning:

Synchronization on local variable 'tileRequest'


Reports synchronization on a local variable or parameter. Such synchronization has little effect, since different threads usually will have different values for the local variable or parameter. The intent of the code will usually be clearer if synchronization on a field is used.


I am not too confident on my understanding of synchronization, waiting and notifying, so can someone tell me if my approach is valid, regardless of the warning?

EDIT
There were some questions stating that no multiple threads were involved, I updated the comments a bit, but the tileProvider.getTile(...) method gets the tile, but there is no guarantee that this will not happen on another thread.

Answer

Synchronizing on a local variable may be ignored by the JVM so you can't count on it having the correct semantics. And it's error prone. I would suggest a simpler approach:

CountDownLatch done = new CountDownLatch(1);
tileProvider.getTile(x, y, zoom, new GetDataListener<byte[]>() {
    @Override
    public void onGetData(byte[] data, Exception exception) {
        tileRequest.data = data;
        tileRequest.exceptionOccurred = exception != null;
        tileRequest.finishedRequest = true;
        done.countDown();
    }
});

done.await();
return tileRequest.getTile();

Note that you probably can't have an if (tileRequest.finishedRequest) return tileRequest.getTile(); before the done.await() because you would lose the visibility guarantee provided by the latch. And based on your code it seems that the latch would be unblocked immediately after finishedRequest is set to true anyway.