CODEWITHSUNDEEP
javaandroidmultithreadingsynchronization
Hylke
Hylke

Reputation: 705

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.

Upvotes: 0

Views: 675

Answers (4)

assylias
assylias

Reputation: 328785

Synchronizing on a local object may be ignored by the JVM in some situations so you can't count on it having the correct semantics without a detailed analysis. 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.

Upvotes: 3

Solomon Slow
Solomon Slow

Reputation: 27190

IMO the compiler warning is over zealous.

Not only that, but it is technically wrong: It is not possible for Java code to synchronize on a variable. Synchronization is something you do to an object.

When you write synchronized (x) { ... }, the x is an expression that is evaluated to yield an object reference. Most programmers expect x to be a private final instance variable or a private final static variable---those are easy cases to understand---but it doesn't have to be that way. x could be a local variable or it could be a method call that returns an object reference.

What's important, is that the object is shared between threads. It doesn't make any sense to synchronize on an object unless some other thread also synchronize on the same object.

The warning message that you're getting is meant to prevent a common newbie mistake. Even if it's technically wrong, it might prevent dozens or hundreds of daily phone calls from confused newbies to some developer help center somewhere.

When I'm working on commercial software, I don't worry as much about whether a decision is "right" or "wrong" as whether it will increase or decrease the number of calls to our support center.

Upvotes: 1

pablochan
pablochan

Reputation: 5715

Your code is correct, albeit clunky. Even though you're using a method-local object for synchronization, it's enclosed by GetDataListener, so it'll potentially be shared by different threads.

Take a look at CompletebleFuture. It might make this code more readable.

Upvotes: 1

sidgate
sidgate

Reputation: 15244

Synchronization is required when two threads tries to access and update same resource, and you want only single thread to access the object at any given time.

In your case, tileRequest is a local variable defined in method scope. So each thread will have it's own instance of the object. So synchronizing local variable will just have an overhead of locking the object without any actual multi-threading benefits.

Upvotes: 1

Related Questions