Limiting concurrent access to a method

I have a problem with limiting concurrent access to a method. I have a method MyService that can be called from many places at many times. This method must return a String, that should be updated according to some rules. For this, I have an updatedString class. Before getting the String, it makes sure that the String is updated, if not, it updates it. Many threads could read the String at the same time but ONLY ONE should renew the String at the same time if it is out of date.

public final class updatedString {

private static final String UPstring;
private static final Object lock = new Object();

public static String getUpdatedString(){
    synchronized(lock){
        if(stringNeedRenewal()){
           renewString();
        }
    }
    return getString();
}

...

This works fine. If I have 7 threads getting the String, it guarantees that, if necessary, ONLY one thread is updating the String.

My question is, is it a good idea to have all this static? Why if not? Is it fast? Is there a better way to do this?

I have read posts like this: What Cases Require Synchronized Method Access in Java? which suggests that static mutable variables are not a good idea, and static classes either. But I cannot see any dead-lock in the code or a better valid solution. Only that some threads will have to wait until the String is updated (if necessary) or wait for other thread to leave the synchronized block (which causes a small delay).

If the method is not static, then I have a problem because this will not work since the synchronized method acts only for the current instance that the thread is using. Synchronized the method does not work either, it seems that the lock instance-specific and not class-specific. The other solution could be to have a Singleton that avoids creatin开发者_开发百科g more than one instance and then use a single synchronized not-static class, but I do not like this solution too much.

Additional information:

stringNeedRenewal() is not too expensive although it has to read from a database. renewString() on the contrary is very expensive, and has to read from several tables on the database to finally come to an answer. The String needs arbitrary renewal, but this does not happen very often (from once per hour to once per week).

@forsvarir made me think... and I think he/she was right. return getString(); MUST be inside the synchronized method. At a first sight it looks as if it can be out of it so threads will be able to read it concurrently, but what happens if a thread stops running WHILE calling getString() and other thread partially execute renewString()? We could have this situation (assuming a single processor):

1. THREAD 1 starts getString(). The OS starts copying into memory the bytes to be returned.

2. THREAD 1 is stopped by the OS before finishing the copy.

3. THREAD 2 enters the synchronized block and starts renewString(), changing the original String in memory.

4. THREAD 1 gets control back and finish getString using a corrupted String!! So it copied one part from the old string and another from the new one.

Having the read inside the synchronized block can make everything very slow, since threads could only access this one by one.

As @Jeremy Heiler pointed out, this is an abstract problem of a cache. If the cache is old, renew it. If not, use it. It is better more clear to picture the problem like this instead of a single String (or imagine that there are 2 strings instead of one). So what happens if someone is reading at the same time as someone is modifying the cache?

First of all, you can remove the lock and the synchronized block and simply use:

public static synchronized String getUpdatedString(){
    if(stringNeedRenewal()){
       renewString();
    }
    return getString();
}

this synchronizes on the UpdatedString.class object.

Another thing you can do is used double-checked locking to prevent unnecessary waiting. Declare the string to be volatile and:

public static String getUpdatedString(){
    if(stringNeedRenewal()){
        synchronized(lock) {
            if(stringNeedRenewal()){
                renewString();
            }
        }
    }
    return getString();
}

Then, whether to use static or not - it seems it should be static, since you want to invoke it without any particular instance.

I would suggest looking into a ReentrantReadWriteLock. (Whether or not it is performant is up to you to decide.) This way you can have many read operations occur simultaneously.

Here is the example from the documentation:

 class CachedData {
   Object data;
   volatile boolean cacheValid;
   ReentrantReadWriteLock rwl = new ReentrantReadWriteLock();

   void processCachedData() {
     rwl.readLock().lock();
     if (!cacheValid) {
        // Must release read lock before acquiring write lock
        rwl.readLock().unlock();
        rwl.writeLock().lock();
        // Recheck state because another thread might have acquired
        //   write lock and changed state before we did.
        if (!cacheValid) {
          data = ...
          cacheValid = true;
        }
        // Downgrade by acquiring read lock before releasing write lock
        rwl.readLock().lock();
        rwl.writeLock().unlock(); // Unlock write, still hold read
     }

     use(data);
     rwl.readLock().unlock();
   }
 }

This isn't exactly what you're after, and I'm not a Java specialist, so take this with a pinch of salt :)

Perhaps the code sample you've provided is contrived, but if not, I'm unclear what the purpose of the class is. You only want one thread to update the string to it's new value. Why? Is it to save effort (because you'd rather use the processor cycles on something else)? Is it to maintain consistentcy (once a certain point is reached, the string must be updated)?

How long is the cycle between required updates?

Looking at your code...

public final class updatedString {

private static final String UPstring;
private static final Object lock = new Object();

public static String getUpdatedString(){
    synchronized(lock){
        // One thread is in this block at a time
        if(stringNeedRenewal()){
           renewString();  // This updates the shared string?
        }
    }
    // At this point, you're calling out to a method.  I don't know what the
    // method does, I'm assuming it just returns UPstring, but at this point, 
    // you're no longer synchronized.  The string actually returned may or may  
    // not be the same one that was present when the thread went through the 
    // synchronized section hence the question, what is the purpose of the
    // synchronization...
    return getString();  // This returns the shared string?
}

The right locking / optimizations depend upon the reason that you're putting them in place, the likelyhood of a write being required and as Paulo has said, the cost of the operations involved.

For some situations where writes are rare, and obviously depending upon what renewString does, it may be desirable to use an optimistic write approach. Where each thread checks if a refresh is required, proceeds to perform the update on a local and then only at the end, assigns the value across to the field being read (you need to track the age of your updates if you follow this approach). This would be faster for reading, since the check for 'does the string need renewed' can be performed outside of the synchronised section. Various other approaches could be used, depending upon the individual scenario...

as long as you lock is static, everything else doesn't have to be, and things will work just as they do now