Ufyukyu

Question

I have a webapp that I am in the middle of doing some load/performance testing on, particularily on a feature where we expect a few hundred users to be accessing the same page and hitting refresh about every 10 seconds on this page. One area of improvement that we found we could make with this function was to cache the responses from the web service for some period of time, since the data is not changing.

After implementing this basic caching, in some further testing I found out that I didn't consider how concurrent threads could access the Cache at the same time. I found that within the matter of ~100ms, about 50 threads were trying to fetch the object from the Cache, finding that it had expired, hitting the web service to fetch the data, and then putting the object back in the cache.

The original code looked something like this:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {



  final String key = "Data-" + email;

  SomeData data = (SomeData) StaticCache.get(key);



  if (data == null) {

      data = service.getSomeDataForEmail(email);



      StaticCache.set(key, data, CACHE_TIME);

  }

  else {

      logger.debug("getSomeDataForEmail: using cached object");

  }



  return data;

}

So, to make sure that only one thread was calling the web service when the object at key expired, I thought I needed to synchronize the Cache get/set operation, and it seemed like using the cache key would be a good candidate for an object to synchronize on (this way, calls to this method for email b@b.com would not be blocked by method calls to a@a.com).

I updated the method to look like this:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {





  SomeData data = null;

  final String key = "Data-" + email;



  synchronized(key) {      

    data =(SomeData) StaticCache.get(key);



    if (data == null) {

        data = service.getSomeDataForEmail(email);

        StaticCache.set(key, data, CACHE_TIME);

    }

    else {

      logger.debug("getSomeDataForEmail: using cached object");

    }

  }



  return data;

}

I also added logging lines for things like "before synchronization block", "inside synchronization block", "about to leave synchronization block", and "after synchronization block", so I could determine if I was effectively synchronizing the get/set operation.

However it doesn't seem like this has worked. My test logs have output like:

(log output is 'threadname' 'logger name' 'message')

http-80-Processor253 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor253 jsp.view-page - getSomeDataForEmail: inside synchronization block

http-80-Processor253 cache.StaticCache - get: object at key [SomeData-test@test.com] has expired

http-80-Processor253 cache.StaticCache - get: key [SomeData-test@test.com] returning value [null]

http-80-Processor263 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor263 jsp.view-page - getSomeDataForEmail: inside synchronization block

http-80-Processor263 cache.StaticCache - get: object at key [SomeData-test@test.com] has expired

http-80-Processor263 cache.StaticCache - get: key [SomeData-test@test.com] returning value [null]

http-80-Processor131 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor131 jsp.view-page - getSomeDataForEmail: inside synchronization block

http-80-Processor131 cache.StaticCache - get: object at key [SomeData-test@test.com] has expired

http-80-Processor131 cache.StaticCache - get: key [SomeData-test@test.com] returning value [null]

http-80-Processor104 jsp.view-page - getSomeDataForEmail: inside synchronization block

http-80-Processor104 cache.StaticCache - get: object at key [SomeData-test@test.com] has expired

http-80-Processor104 cache.StaticCache - get: key [SomeData-test@test.com] returning value [null]

http-80-Processor252 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor283 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor2 jsp.view-page - getSomeDataForEmail: about to enter synchronization block

http-80-Processor2 jsp.view-page - getSomeDataForEmail: inside synchronization block

I wanted to see only one thread at a time entering/exiting the synchronization block around the get/set operations.

Is there an issue in synchronizing on String objects? I thought the cache-key would be a good choice as it is unique to the operation, and even though the final String key is declared within the method, I was thinking that each thread would be getting a reference to the same object and therefore would synchronization on this single object.

What am I doing wrong here?

Update: after looking further at the logs, it seems like methods with the same synchronization logic where the key is always the same, such as

final String key = "blah";

...

synchronized(key) { ...

do not exhibit the same concurrency problem - only one thread at a time is entering the block.

Update 2: Thanks to everyone for the help! I accepted the first answer about intern()ing Strings, which solved my initial problem - where multiple threads were entering synchronized blocks where I thought they shouldn't, because the key's had the same value.

As others have pointed out, using intern() for such a purpose and synchronizing on those Strings does indeed turn out to be a bad idea - when running JMeter tests against the webapp to simulate the expected load, I saw the used heap size grow to almost 1GB in just under 20 minutes.

Currently I'm using the simple solution of just synchronizing the entire method - but I really like the code samples provided by martinprobst and MBCook, but since I have about 7 similar getData() methods in this class currently (since it needs about 7 different pieces of data from a web service), I didn't want to add almost-duplicate logic about getting and releasing locks to each method. But this is definitely very, very valuable info for future usage. I think these are ultimately the correct answers on how best to make an operation like this thread-safe, and I'd give out more votes to these answers if I could!

You no longer need to worry about the intern String hanging around in memory - Apparently intern'd strings have been GC'd for quite some time now: stackoverflow.com/questions/18152560/… — Feb 16 '17 at 17:07
I recommend this answer, usng Guava's Striped<Lock> to avoid excessive memory usage: stackoverflow.com/a/11125602/116810 — Feb 24 '18 at 0:09

score 38 · Accepted Answer · 2008-09-26 12:37:48Z

Without putting my brain fully into gear, from a quick scan of what you say it looks as though you need to intern() your Strings:

final String firstkey = "Data-" + email;

final String key = firstkey.intern();

Two Strings with the same value are otherwise not necessarily the same object.

Note that this may introduce a new point of contention, since deep in the VM, intern() may have to acquire a lock. I have no idea what modern VMs look like in this area, but one hopes they are fiendishly optimised.

I assume you know that StaticCache still needs to be thread-safe. But the contention there should be tiny compared with what you'd have if you were locking on the cache rather than just the key while calling getSomeDataForEmail.

Response to question update:

I think that's because a string literal always yields the same object. Dave Costa points out in a comment that it's even better than that: a literal always yields the canonical representation. So all String literals with the same value anywhere in the program would yield the same object.

Edit

Others have pointed out that synchronizing on intern strings is actually a really bad idea - partly because creating intern strings is permitted to cause them to exist in perpetuity, and partly because if more than one bit of code anywhere in your program synchronizes on intern strings, you have dependencies between those bits of code, and preventing deadlocks or other bugs may be impossible.

Strategies to avoid this by storing a lock object per key string are being developed in other answers as I type.

Here's an alternative - it still uses a singular lock, but we know we're going to need one of those for the cache anyway, and you were talking about 50 threads, not 5000, so that may not be fatal. I'm also assuming that the performance bottleneck here is slow blocking I/O in DoSlowThing() which will therefore hugely benefit from not being serialised. If that's not the bottleneck, then:

If the CPU is busy then this approach may not be sufficient and you need another approach.

If the CPU is not busy, and access to server is not a bottleneck, then this approach is overkill, and you might as well forget both this and per-key locking, put a big synchronized(StaticCache) around the whole operation, and do it the easy way.

Obviously this approach needs to be soak tested for scalability before use -- I guarantee nothing.

This code does NOT require that StaticCache is synchronized or otherwise thread-safe. That needs to be revisited if any other code (for example scheduled clean-up of old data) ever touches the cache.

IN_PROGRESS is a dummy value - not exactly clean, but the code's simple and it saves having two hashtables. It doesn't handle InterruptedException because I don't know what your app wants to do in that case. Also, if DoSlowThing() consistently fails for a given key this code as it stands is not exactly elegant, since every thread through will retry it. Since I don't know what the failure criteria are, and whether they are liable to be temporary or permanent, I don't handle this either, I just make sure threads don't block forever. In practice you may want to put a data value in the cache which indicates 'not available', perhaps with a reason, and a timeout for when to retry.

// do not attempt double-check locking here. I mean it.

synchronized(StaticObject) {

    data = StaticCache.get(key);

    while (data == IN_PROGRESS) {

        // another thread is getting the data

        StaticObject.wait();

        data = StaticCache.get(key);

    }

    if (data == null) {

        // we must get the data

        StaticCache.put(key, IN_PROGRESS, TIME_MAX_VALUE);

    }

}

if (data == null) {

    // we must get the data

    try {

        data = server.DoSlowThing(key);

    } finally {

        synchronized(StaticObject) {

            // WARNING: failure here is fatal, and must be allowed to terminate

            // the app or else waiters will be left forever. Choose a suitable

            // collection type in which replacing the value for a key is guaranteed.

            StaticCache.put(key, data, CURRENT_TIME);

            StaticObject.notifyAll();

        }

    }

}

Every time anything is added to the cache, all threads wake up and check the cache (no matter what key they're after), so it's possible to get better performance with less contentious algorithms. However, much of that work will take place during your copious idle CPU time blocking on I/O, so it may not be a problem.

This code could be commoned-up for use with multiple caches, if you define suitable abstractions for the cache and its associated lock, the data it returns, the IN_PROGRESS dummy, and the slow operation to perform. Rolling the whole thing into a method on the cache might not be a bad idea.

quite right, and the reason it works when you have a constant key is that string literals are automatically intern'd. — Sep 25 '08 at 15:39
Ah, that answers my vague 'I can't remember' ramble. Thanks. — Sep 25 '08 at 15:42
Calling String.intern() on a web server where the String cache might live indefinitely is not a good idea. Not saying this post is incorrect, but it is not a good solution to the original question. — Sep 25 '08 at 16:23
I agree. I'll mention it, and I've asked martinprobst below whether he wants to do anything about merging the various relevant points. — Sep 25 '08 at 16:35
I think this is a very nice solution. From an algorithmic standpoint, it's quite similar to the table of locks method, the difference being that you use the static cache as the lock-table, and do not actually store locks but just the flag. So, this is the best solution. — Sep 26 '08 at 8:23

score 25 · Accepted Answer · 2008-09-25 16:17:17Z

Synchronizing on an intern'd String might not be a good idea at all - by interning it, the String turns into a global object, and if you synchronize on the same interned strings in different parts of your application, you might get really weird and basically undebuggable synchronization issues such as deadlocks. It might seem unlikely, but when it happens you are really screwed. As a general rule, only ever synchronize on a local object where you're absolutely sure that no code outside of your module might lock it.

In your case, you can use a synchronized hashtable to store locking objects for your keys.

E.g.:

Object data = StaticCache.get(key, ...);

if (data == null) {

  Object lock = lockTable.get(key);

  if (lock == null) {

    // we're the only one looking for this

    lock = new Object();

    synchronized(lock) {

      lockTable.put(key, lock);

      // get stuff

      lockTable.remove(key);

    }

  } else {

    synchronized(lock) {

      // just to wait for the updater

    }

    data = StaticCache.get(key);

  }

} else {

  // use from cache

}

This code has a race condition, where two threads might put an object into the lock table after each other. This should however not be a problem, because then you only have one more thread calling the webservice and updating the cache, which shouldn't be a problem.

If you're invalidating the cache after some time, you should check whether data is null again after retrieving it from the cache, in the lock != null case.

Alternatively, and much easier, you can make the whole cache lookup method ("getSomeDataByEmail") synchronized. This will mean that all threads have to synchronize when they access the cache, which might be a performance problem. But as always, try this simple solution first and see if it's really a problem! In many cases it should not be, as you probably spend much more time processing the result than synchronizing.

Question - do you mean if I synchronize on the SAME interned string elsewhere in the application? — Sep 25 '08 at 15:59
Agreed. Props for pointing out the problem and providing a pretty good solution. Maybe edit this response to include actual code to seal the deal? — Sep 25 '08 at 16:06
Would it be theft if I incorporated this into my answer, to give a single complete criticism? You deserve the rep points, though, for the most advanced answer, so feel free to turn yours into the all-singing all-dancing. — Sep 25 '08 at 16:16
Looks like this example has a problem where the access to lockTable isn't synchronized, which basically can have at least two bad consequences: (1) you might end up performing multiple concurrent operations on StaticCache for the same key, (2) different states of lockTable seen by different CPUs. — Sep 25 '08 at 16:41

score 9 · Accepted Answer · 2008-12-03 13:34:13Z

Strings are not good candidates for synchronization. If you must synchronize on a String ID, it can be done by using the string to create a mutex (see "synchronizing on an ID"). Whether the cost of that algorithm is worth it depends on whether invoking your service involves any significant I/O.

Also:

I hope the StaticCache.get() and set() methods are threadsafe.

String.intern() comes at a cost (one that varies between VM implementations) and should be used with care.

MBCookMBCook 12.4k63340 · Accepted Answer · 2008-09-25 16:04:40Z

Others have suggested interning the strings, and that will work.

The problem is that Java has to keep interned strings around. I was told it does this even if you're not holding a reference because the value needs to be the same the next time someone uses that string. This means interning all the strings may start eating up memory, which with the load you're describing could be a big problem.

I have seen two solutions to this:

You could synchronize on another object

Instead of the email, make an object that holds the email (say the User object) that holds the value of email as a variable. If you already have another object that represents the person (say you already pulled something from the DB based on their email) you could use that. By implementing the equals method and the hashcode method you can make sure Java considers the objects the same when you do a static cache.contains() to find out if the data is already in the cache (you'll have to synchronize on the cache).

Actually, you could keep a second Map for objects to lock on. Something like this:

Map<String, Object> emailLocks = new HashMap<String, Object>();



Object lock = null;



synchronized (emailLocks) {

    lock = emailLocks.get(emailAddress);



    if (lock == null) {

        lock = new Object();

        emailLocks.put(emailAddress, lock);

    }

}



synchronized (lock) {

    // See if this email is in the cache

    // If so, serve that

    // If not, generate the data



    // Since each of this person's threads synchronizes on this, they won't run

    // over eachother. Since this lock is only for this person, it won't effect

    // other people. The other synchronized block (on emailLocks) is small enough

    // it shouldn't cause a performance problem.

}

This will prevent 15 fetches on the same email address at one. You'll need something to prevent too many entries from ending up in the emailLocks map. Using LRUMaps from Apache Commons would do it.

This will need some tweaking, but it may solve your problem.

Use a different key

If you are willing to put up with possible errors (I don't know how important this is) you could use the hashcode of the String as the key. ints don't need to be interned.

Summary

I hope this helps. Threading is fun, isn't it? You could also use the session to set a value meaning "I'm already working on finding this" and check that to see if the second (third, Nth) thread needs to attempt to create the or just wait for the result to show up in the cache. I guess I had three suggestions.

Your emailLocks leaks - this approach needs some sort of reference count and release mechanism. — Sep 25 '08 at 16:21
"ints don't need to be interned" - but don't have monitors. Your summary hints at the exciting world of wait/notifyAll, which is always a laugh :-) — Sep 25 '08 at 16:24
@McDowell: I think a WeakHashMap might actually be appropriate here. However, that's such a rare occurrence than I'm hesitant to say any more... — Sep 25 '08 at 16:32
@McDowell: That's why I said the LRUMap in the comments, it would prevent you from ever having more than say 30 entries. You could also setup a thread to do it. — Sep 25 '08 at 18:19
@onebyone: Good point. Integers do, but an Integer doesn't equal an Integer like int, you have to use .intValue() or .equals(). LIke I said, they were quick suggestions. I would have caught that when I noticed the code didn't work :) — Sep 25 '08 at 18:20

oxbow_lakesoxbow_lakes 108k48291433 · Accepted Answer · 2008-09-27 17:26:01Z

You can use the 1.5 concurrency utilities to provide a cache designed to allow multiple concurrent access, and a single point of addition (i.e. only one thread ever performing the expensive object "creation"):

 private ConcurrentMap<String, Future<SomeData> cache;

 private SomeData getSomeDataByEmail(final WebServiceInterface service, final String email) throws Exception {



  final String key = "Data-" + email;

  Callable<SomeData> call = new Callable<SomeData>() {

      public SomeData call() {

          return service.getSomeDataForEmail(email);

      }

  }

  FutureTask<SomeData> ft; ;

  Future<SomeData> f = cache.putIfAbsent(key, ft= new FutureTask<SomeData>(call)); //atomic

  if (f == null) { //this means that the cache had no mapping for the key

      f = ft;

      ft.run();

  }

  return f.get(); //wait on the result being available if it is being calculated in another thread

}

Obviously, this doesn't handle exceptions as you'd want to, and the cache doesn't have eviction built in. Perhaps you could use it as a basis to change your StaticCache class, though.

Liking this solution a lot - no need to keep a separate in-progress sentinel or separate map of keys to locks - it either contains the Future (another thread is working on it, just call get), or it doesn't (no other threads working on it, do the expensive calculation) — Nov 2 '16 at 1:17

kohlermkohlerm 2,25811417 · Accepted Answer · 2008-10-08 08:48:10Z

Use a decent caching framework such as ehcache.

Implementing a good cache is not as easy as some people believe.

Regarding the comment that String.intern() is a source of memory leaks, that is actually not true.
Interned Strings are garbage collected,it just might take longer because on certain JVM'S (SUN) they are stored in Perm space which is only touched by full GC's.

score 3 · Accepted Answer · 2019-01-01 22:58:38Z

Here is a safe short Java 8 solution that uses a map of dedicated lock objects for synchronization:

private static final Map<String, Object> keyLocks = new ConcurrentHashMap<>();



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {

        SomeData data = StaticCache.get(key);

        if (data == null) {

            data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data);

        }

    }

    return data;

}

It has a drawback that keys and lock objects would retain in map forever.

This can be worked around like this:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {

        try {

            SomeData data = StaticCache.get(key);

            if (data == null) {

                data = service.getSomeDataForEmail(email);

                StaticCache.set(key, data);

            }

        } finally {

            keyLocks.remove(key); // vulnerable to race-conditions

        }

    }

    return data;

}

But then popular keys would be constantly reinserted in map with lock objects being reallocated.

Update: And this leaves race condition possibility when two threads would concurrently enter synchronized section for the same key but with different locks.

So it may be more safe and efficient to use expiring Guava Cache:

private static final LoadingCache<String, Object> keyLocks = CacheBuilder.newBuilder()

        .expireAfterAccess(10, TimeUnit.MINUTES) // max lock time ever expected

        .build(CacheLoader.from(Object::new));



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.getUnchecked(key)) {

        SomeData data = StaticCache.get(key);

        if (data == null) {

            data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data);

        }

    }

    return data;

}

Note that it's assumed here that StaticCache is thread-safe and wouldn't suffer from concurrent reads and writes for different keys.

You could in theory use the calculated key as lock-object, then you avoid re-allocations of new objects (the key needs to be calculated either way). If a ConcurrentHashMap is used for keyLocks that shouldn't leave room for race conditions? computeIfAbsent would only insert the first one, later attempts would return the identical instance? — Dec 30 '18 at 10:16
@knittl, yes, but it would not be reliable without intern(). And it would suffer of all downsides mentioned in Martin Probst's answer here. — Dec 30 '18 at 17:35
why would it not be reliable without interning the string? As far as I see all synchronize block would use the same string instance (because it is put in the map and reused). The string is local, because it is concatenated inside the method. — Dec 31 '18 at 10:22
@knittl, there is no guarantee that subsequent concatenations would return the same object. And there is no guarantee that concatenated string would not be the same as interned instance that may be synchronized in some unrelated place elsewhere. — Jan 1 at 21:18
You don't need identical objects. You are using the concatenated strings as key in the hashmap. They provide hash and value equality, so you will find the correct value for equal keys. But having strings auto-interned is a good point! (I doubt it though, because the emitted byte code is usually a regular string builder (or fancy invokedynamic code in newer JDK versions)) — Jan 1 at 21:43

AlexanderAlexander 7,20321921 · Accepted Answer · 2008-09-25 15:39:28Z

2

Your main problem is not just that there might be multiple instances of String with the same value. The main problem is that you need to have only one monitor on which to synchronize for accessing the StaticCache object. Otherwise multiple threads might end up concurrently modifying StaticCache (albeit under different keys), which most likely doesn't support concurrent modification.

answered Sep 25 '08 at 15:39

Alexander

7,20321921

That's solved by making it use, for example, ConcurrentHashmap.

– Steve Jessop
Sep 25 '08 at 16:08

You're right, I forgot about ConcurrentHashmap! However, judging by the update of the question, StaticCache doesn't really support concurrent modification.

– Alexander
Sep 25 '08 at 16:37

add a comment |

score 2 · Accepted Answer · 2008-09-25 15:46:21Z

The call:

   final String key = "Data-" + email;

creates a new object every time the method is called. Because that object is what you use to lock, and every call to this method creates a new object, then you are not really synchronizing access to the map based on the key.

This further explain your edit. When you have a static string, then it will work.

Using intern() solves the problem, because it returns the string from an internal pool kept by the String class, that ensures that if two strings are equal, the one in the pool will be used. See

http://java.sun.com/j2se/1.4.2/docs/api/java/lang/String.html#intern()

igor.zhigor.zh 626710 · Accepted Answer · 2018-05-11 18:31:56Z

This question seems to me a bit too broad, and therefore it instigated equally broad set of answers. So I'll try to answer the question I have been redirected from, unfortunately that one has been closed as duplicate.

public class ValueLock<T> {



    private Lock lock = new ReentrantLock();

    private Map<T, Condition> conditions  = new HashMap<T, Condition>();



    public void lock(T t){

        lock.lock();

        try {

            while (conditions.containsKey(t)){

                conditions.get(t).awaitUninterruptibly();

            }

            conditions.put(t, lock.newCondition());

        } finally {

            lock.unlock();

        }

    }



    public void unlock(T t){

        lock.lock();

        try {

            Condition condition = conditions.get(t);

            if (condition == null)

                throw new IllegalStateException();// possibly an attempt to release what wasn't acquired

            conditions.remove(t);

            condition.signalAll();

        } finally {

            lock.unlock();

        }

    }

Upon the (outer) lock operation the (inner) lock is acquired to get an exclusive access to the map for a short time, and if the correspondent object is already in the map, the current thread will wait,
otherwise it will put new Condition to the map, release the (inner) lock and proceed,
and the (outer) lock is considered obtained.
The (outer) unlock operation, first acquiring an (inner) lock, will signal on Condition and then remove the object from the map.

The class does not use concurrent version of Map, because every access to it is guarded by single (inner) lock.

Please notice, the semantic of lock() method of this class is different that of ReentrantLock.lock(), the repeated lock() invocations without paired unlock() will hang current thread indefinitely.

An example of usage that might be applicable to the situation, the OP described

    ValueLock<String> lock = new ValueLock<String>();

    // ... share the lock   

    String email = "...";

    try {

        lock.lock(email);

        //... 

    } finally {

        lock.unlock(email);

    }

I'm very interested by your ValueLock object, but confused by it. The way I read it, if Thread 2 calls` lock()` while Thread1 is already doing something, it will wait at the conditions.awaitUninteruptibly(). Which will block T1 from calling the unlock() method since the ReentrantLock is being held by T2 at that point. Am I mis-reading something? — Nov 27 '18 at 23:22
@Eric B, Neither of Threads 1 or 2 of yours holds an inner (Reentrant) lock except for very short period of time while operating Value-to-Condition map. When Thread 1 holds an Value lock it means it holds a lock on certain Value-bound Condition, not on inner (Reentrant) Lock. When Thread 2 awaitsUninterruptibly on this Value-bound Condition it actually releases the inner(Reentrant) Lock. Until it acquires this Condition, then acquires an inner (Reentrant) Lock, puts the Condition into map and immediately releases inner (Reentrant) Lock. Please let me know if I will have to rephrase it — Nov 29 '18 at 19:28

smerny 13.2k185069 · Accepted Answer · 2013-08-22 19:34:17Z

This is rather late, but there is quite a lot of incorrect code presented here.

In this example:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {





  SomeData data = null;

  final String key = "Data-" + email;



  synchronized(key) {      

    data =(SomeData) StaticCache.get(key);



    if (data == null) {

        data = service.getSomeDataForEmail(email);

        StaticCache.set(key, data, CACHE_TIME);

    }

    else {

      logger.debug("getSomeDataForEmail: using cached object");

    }

  }



  return data;

}

The synchronization is incorrectly scoped. For a static cache that supports a get/put API, there should be at least synchronization around the get and getIfAbsentPut type operations, for safe access to the cache. The scope of synchronization will be the cache itself.

If updates must be made to the data elements themselves, that adds an additional layer of synchronization, which should be on the individual data elements.

SynchronizedMap can be used in place of explicit synchronization, but care must still be observed. If the wrong APIs are used (get and put instead of putIfAbsent) then the operations won't have the necessary synchronization, despite the use of the synchronized map. Notice the complications introduced by the use of putIfAbsent: Either, the put value must be computed even in cases when it is not needed (because the put cannot know if the put value is needed until the cache contents are examined), or requires a careful use of delegation (say, using Future, which works, but is somewhat of a mismatch; see below), where the put value is obtained on demand if needed.

The use of Futures is possible, but seems rather awkward, and perhaps a bit of overengineering. The Future API is at it's core for asynchronous operations, in particular, for operations which may not complete immediately. Involving Future very probably adds a layer of thread creation -- extra probably unnecessary complications.

The main problem of using Future for this type of operation is that Future inherently ties in multi-threading. Use of Future when a new thread is not necessary means ignoring a lot of the machinery of Future, making it an overly heavy API for this use.

MattW.MattW. 10.2k44660 · Accepted Answer · 2008-09-25 15:59:34Z

0

Why not just render a static html page that gets served to the user and regenerated every x minutes?

answered Sep 25 '08 at 15:59

MattW.

10.2k44660

add a comment |

John GardnerJohn Gardner 19.5k34163 · Accepted Answer · 2008-09-25 19:06:59Z

I'd also suggest getting rid of the string concatenation entirely if you don't need it.

final String key = "Data-" + email;

Is there other things/types of objects in the cache that use the email address that you need that extra "Data-" at the beginning of the key?

if not, i'd just make that

final String key = email;

and you avoid all that extra string creation too.

celencelen 11 · Accepted Answer · 2016-10-12 02:33:27Z

0

other way synchronizing on string object :

String cacheKey = ...;



    Object obj = cache.get(cacheKey)



    if(obj==null){

    synchronized (Integer.valueOf(Math.abs(cacheKey.hashCode()) % 127)){

          obj = cache.get(cacheKey)

         if(obj==null){

             //some cal obtain obj value,and put into cache

        }

    }

}

answered Oct 12 '16 at 2:33

celen

11

This has the same drawbacks as String.intern with higher probability of getting into trouble.

– Vadzim
Nov 11 '17 at 21:41

add a comment |

score 0 · Accepted Answer · 2017-11-26 20:25:46Z

In case others have a similar problem, the following code works, as far as I can tell:

import java.util.Map;

import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.atomic.AtomicInteger;

import java.util.function.Supplier;



public class KeySynchronizer<T> {



    private Map<T, CounterLock> locks = new ConcurrentHashMap<>();



    public <U> U synchronize(T key, Supplier<U> supplier) {

        CounterLock lock = locks.compute(key, (k, v) -> 

                v == null ? new CounterLock() : v.increment());

        synchronized (lock) {

            try {

                return supplier.get();

            } finally {

                if (lock.decrement() == 0) {

                    // Only removes if key still points to the same value,

                    // to avoid issue described below.

                    locks.remove(key, lock);

                }

            }

        }

    }



    private static final class CounterLock {



        private AtomicInteger remaining = new AtomicInteger(1);



        private CounterLock increment() {

            // Returning a new CounterLock object if remaining = 0 to ensure that

            // the lock is not removed in step 5 of the following execution sequence:

            // 1) Thread 1 obtains a new CounterLock object from locks.compute (after evaluating "v == null" to true)

            // 2) Thread 2 evaluates "v == null" to false in locks.compute

            // 3) Thread 1 calls lock.decrement() which sets remaining = 0

            // 4) Thread 2 calls v.increment() in locks.compute

            // 5) Thread 1 calls locks.remove(key, lock)

            return remaining.getAndIncrement() == 0 ? new CounterLock() : this;

        }



        private int decrement() {

            return remaining.decrementAndGet();

        }

    }

}

In the case of the OP, it would be used like this:

private KeySynchronizer<String> keySynchronizer = new KeySynchronizer<>();



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    String key = "Data-" + email;

    return keySynchronizer.synchronize(key, () -> {

        SomeData existing = (SomeData) StaticCache.get(key);

        if (existing == null) {

            SomeData data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data, CACHE_TIME);

            return data;

        }

        logger.debug("getSomeDataForEmail: using cached object");

        return existing;

    });

}

If nothing should be returned from the synchronized code, the synchronize method can be written like this:

public void synchronize(T key, Runnable runnable) {

    CounterLock lock = locks.compute(key, (k, v) -> 

            v == null ? new CounterLock() : v.increment());

    synchronized (lock) {

        try {

            runnable.run();

        } finally {

            if (lock.decrement() == 0) {

                // Only removes if key still points to the same value,

                // to avoid issue described below.

                locks.remove(key, lock);

            }

        }

    }

}

score 0 · Accepted Answer · 2018-05-27 13:33:44Z

I've added a small lock class that can lock/synchronize on any key, including strings.

See implementation for Java 8, Java 6 and a small test.

Java 8:

public class DynamicKeyLock<T> implements Lock

{

    private final static ConcurrentHashMap<Object, LockAndCounter> locksMap = new ConcurrentHashMap<>();



    private final T key;



    public DynamicKeyLock(T lockKey)

    {

        this.key = lockKey;

    }



    private static class LockAndCounter

    {

        private final Lock lock = new ReentrantLock();

        private final AtomicInteger counter = new AtomicInteger(0);

    }



    private LockAndCounter getLock()

    {

        return locksMap.compute(key, (key, lockAndCounterInner) ->

        {

            if (lockAndCounterInner == null) {

                lockAndCounterInner = new LockAndCounter();

            }

            lockAndCounterInner.counter.incrementAndGet();

            return lockAndCounterInner;

        });

    }



    private void cleanupLock(LockAndCounter lockAndCounterOuter)

    {

        if (lockAndCounterOuter.counter.decrementAndGet() == 0)

        {

            locksMap.compute(key, (key, lockAndCounterInner) ->

            {

                if (lockAndCounterInner == null || lockAndCounterInner.counter.get() == 0) {

                    return null;

                }

                return lockAndCounterInner;

            });

        }

    }



    @Override

    public void lock()

    {

        LockAndCounter lockAndCounter = getLock();



        lockAndCounter.lock.lock();

    }



    @Override

    public void unlock()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);

        lockAndCounter.lock.unlock();



        cleanupLock(lockAndCounter);

    }





    @Override

    public void lockInterruptibly() throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        try

        {

            lockAndCounter.lock.lockInterruptibly();

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }

    }



    @Override

    public boolean tryLock()

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired = lockAndCounter.lock.tryLock();



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired;

        try

        {

            acquired = lockAndCounter.lock.tryLock(time, unit);

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public Condition newCondition()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);



        return lockAndCounter.lock.newCondition();

    }

}

Java 6:

public class DynamicKeyLock implements Lock
{
private final static ConcurrentHashMap locksMap = new ConcurrentHashMap();
private final T key;

    public DynamicKeyLock(T lockKey) {

        this.key = lockKey;

    }



    private static class LockAndCounter {

        private final Lock lock = new ReentrantLock();

        private final AtomicInteger counter = new AtomicInteger(0);

    }



    private LockAndCounter getLock()

    {

        while (true) // Try to init lock

        {

            LockAndCounter lockAndCounter = locksMap.get(key);



            if (lockAndCounter == null)

            {

                LockAndCounter newLock = new LockAndCounter();

                lockAndCounter = locksMap.putIfAbsent(key, newLock);



                if (lockAndCounter == null)

                {

                    lockAndCounter = newLock;

                }

            }



            lockAndCounter.counter.incrementAndGet();



            synchronized (lockAndCounter)

            {

                LockAndCounter lastLockAndCounter = locksMap.get(key);

                if (lockAndCounter == lastLockAndCounter)

                {

                    return lockAndCounter;

                }

                // else some other thread beat us to it, thus try again.

            }

        }

    }



    private void cleanupLock(LockAndCounter lockAndCounter)

    {

        if (lockAndCounter.counter.decrementAndGet() == 0)

        {

            synchronized (lockAndCounter)

            {

                if (lockAndCounter.counter.get() == 0)

                {

                    locksMap.remove(key);

                }

            }

        }

    }



    @Override

    public void lock()

    {

        LockAndCounter lockAndCounter = getLock();



        lockAndCounter.lock.lock();

    }



    @Override

    public void unlock()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);

        lockAndCounter.lock.unlock();



        cleanupLock(lockAndCounter);

    }





    @Override

    public void lockInterruptibly() throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        try

        {

            lockAndCounter.lock.lockInterruptibly();

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }

    }



    @Override

    public boolean tryLock()

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired = lockAndCounter.lock.tryLock();



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired;

        try

        {

            acquired = lockAndCounter.lock.tryLock(time, unit);

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public Condition newCondition()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);



        return lockAndCounter.lock.newCondition();

    }

}

Test:

public class DynamicKeyLockTest

{

    @Test

    public void testDifferentKeysDontLock() throws InterruptedException

    {

        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(new Object());

        lock.lock();

        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);

        try

        {

            new Thread(() ->

            {

                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(new Object());

                anotherLock.lock();

                try

                {

                    anotherThreadWasExecuted.set(true);

                }

                finally

                {

                    anotherLock.unlock();

                }

            }).start();

            Thread.sleep(100);

        }

        finally

        {

            Assert.assertTrue(anotherThreadWasExecuted.get());

            lock.unlock();

        }

    }



    @Test

    public void testSameKeysLock() throws InterruptedException

    {

        Object key = new Object();

        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(key);

        lock.lock();

        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);

        try

        {

            new Thread(() ->

            {

                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(key);

                anotherLock.lock();

                try

                {

                    anotherThreadWasExecuted.set(true);

                }

                finally

                {

                    anotherLock.unlock();

                }

            }).start();

            Thread.sleep(100);

        }

        finally

        {

            Assert.assertFalse(anotherThreadWasExecuted.get());

            lock.unlock();

        }

    }

}

marc_s 581k13011221268 · Accepted Answer · 2019-03-08 21:49:39Z

You can safely use String.intern for synchronize if you can reasonably guarantee that the string value is unique across your system. UUIDS are a good way to approach this. You can associate a UUID with your actual string key, either via a cache, a map, or maybe even store the uuid as a field on your entity object.

    @Service   

    public class MySyncService{



      public Map<String, String> lockMap=new HashMap<String, String>();



      public void syncMethod(String email) {



        String lock = lockMap.get(email);

        if(lock==null) {

            lock = UUID.randomUUID().toString();

            lockMap.put(email, lock);

        }   



        synchronized(lock.intern()) {

                //do your sync code here

        }

    }

score 38 · Accepted Answer · 2008-09-26 12:37:48Z

Without putting my brain fully into gear, from a quick scan of what you say it looks as though you need to intern() your Strings:

final String firstkey = "Data-" + email;

final String key = firstkey.intern();

Two Strings with the same value are otherwise not necessarily the same object.

Note that this may introduce a new point of contention, since deep in the VM, intern() may have to acquire a lock. I have no idea what modern VMs look like in this area, but one hopes they are fiendishly optimised.

I assume you know that StaticCache still needs to be thread-safe. But the contention there should be tiny compared with what you'd have if you were locking on the cache rather than just the key while calling getSomeDataForEmail.

Response to question update:

I think that's because a string literal always yields the same object. Dave Costa points out in a comment that it's even better than that: a literal always yields the canonical representation. So all String literals with the same value anywhere in the program would yield the same object.

Edit

Others have pointed out that synchronizing on intern strings is actually a really bad idea - partly because creating intern strings is permitted to cause them to exist in perpetuity, and partly because if more than one bit of code anywhere in your program synchronizes on intern strings, you have dependencies between those bits of code, and preventing deadlocks or other bugs may be impossible.

Strategies to avoid this by storing a lock object per key string are being developed in other answers as I type.

Here's an alternative - it still uses a singular lock, but we know we're going to need one of those for the cache anyway, and you were talking about 50 threads, not 5000, so that may not be fatal. I'm also assuming that the performance bottleneck here is slow blocking I/O in DoSlowThing() which will therefore hugely benefit from not being serialised. If that's not the bottleneck, then:

If the CPU is busy then this approach may not be sufficient and you need another approach.

If the CPU is not busy, and access to server is not a bottleneck, then this approach is overkill, and you might as well forget both this and per-key locking, put a big synchronized(StaticCache) around the whole operation, and do it the easy way.

Obviously this approach needs to be soak tested for scalability before use -- I guarantee nothing.

This code does NOT require that StaticCache is synchronized or otherwise thread-safe. That needs to be revisited if any other code (for example scheduled clean-up of old data) ever touches the cache.

IN_PROGRESS is a dummy value - not exactly clean, but the code's simple and it saves having two hashtables. It doesn't handle InterruptedException because I don't know what your app wants to do in that case. Also, if DoSlowThing() consistently fails for a given key this code as it stands is not exactly elegant, since every thread through will retry it. Since I don't know what the failure criteria are, and whether they are liable to be temporary or permanent, I don't handle this either, I just make sure threads don't block forever. In practice you may want to put a data value in the cache which indicates 'not available', perhaps with a reason, and a timeout for when to retry.

// do not attempt double-check locking here. I mean it.

synchronized(StaticObject) {

    data = StaticCache.get(key);

    while (data == IN_PROGRESS) {

        // another thread is getting the data

        StaticObject.wait();

        data = StaticCache.get(key);

    }

    if (data == null) {

        // we must get the data

        StaticCache.put(key, IN_PROGRESS, TIME_MAX_VALUE);

    }

}

if (data == null) {

    // we must get the data

    try {

        data = server.DoSlowThing(key);

    } finally {

        synchronized(StaticObject) {

            // WARNING: failure here is fatal, and must be allowed to terminate

            // the app or else waiters will be left forever. Choose a suitable

            // collection type in which replacing the value for a key is guaranteed.

            StaticCache.put(key, data, CURRENT_TIME);

            StaticObject.notifyAll();

        }

    }

}

Every time anything is added to the cache, all threads wake up and check the cache (no matter what key they're after), so it's possible to get better performance with less contentious algorithms. However, much of that work will take place during your copious idle CPU time blocking on I/O, so it may not be a problem.

This code could be commoned-up for use with multiple caches, if you define suitable abstractions for the cache and its associated lock, the data it returns, the IN_PROGRESS dummy, and the slow operation to perform. Rolling the whole thing into a method on the cache might not be a bad idea.

quite right, and the reason it works when you have a constant key is that string literals are automatically intern'd. — Sep 25 '08 at 15:39
Ah, that answers my vague 'I can't remember' ramble. Thanks. — Sep 25 '08 at 15:42
Calling String.intern() on a web server where the String cache might live indefinitely is not a good idea. Not saying this post is incorrect, but it is not a good solution to the original question. — Sep 25 '08 at 16:23
I agree. I'll mention it, and I've asked martinprobst below whether he wants to do anything about merging the various relevant points. — Sep 25 '08 at 16:35
I think this is a very nice solution. From an algorithmic standpoint, it's quite similar to the table of locks method, the difference being that you use the static cache as the lock-table, and do not actually store locks but just the flag. So, this is the best solution. — Sep 26 '08 at 8:23

score 25 · Accepted Answer · 2008-09-25 16:17:17Z

Synchronizing on an intern'd String might not be a good idea at all - by interning it, the String turns into a global object, and if you synchronize on the same interned strings in different parts of your application, you might get really weird and basically undebuggable synchronization issues such as deadlocks. It might seem unlikely, but when it happens you are really screwed. As a general rule, only ever synchronize on a local object where you're absolutely sure that no code outside of your module might lock it.

In your case, you can use a synchronized hashtable to store locking objects for your keys.

E.g.:

Object data = StaticCache.get(key, ...);

if (data == null) {

  Object lock = lockTable.get(key);

  if (lock == null) {

    // we're the only one looking for this

    lock = new Object();

    synchronized(lock) {

      lockTable.put(key, lock);

      // get stuff

      lockTable.remove(key);

    }

  } else {

    synchronized(lock) {

      // just to wait for the updater

    }

    data = StaticCache.get(key);

  }

} else {

  // use from cache

}

This code has a race condition, where two threads might put an object into the lock table after each other. This should however not be a problem, because then you only have one more thread calling the webservice and updating the cache, which shouldn't be a problem.

If you're invalidating the cache after some time, you should check whether data is null again after retrieving it from the cache, in the lock != null case.

Alternatively, and much easier, you can make the whole cache lookup method ("getSomeDataByEmail") synchronized. This will mean that all threads have to synchronize when they access the cache, which might be a performance problem. But as always, try this simple solution first and see if it's really a problem! In many cases it should not be, as you probably spend much more time processing the result than synchronizing.

Question - do you mean if I synchronize on the SAME interned string elsewhere in the application? — Sep 25 '08 at 15:59
Agreed. Props for pointing out the problem and providing a pretty good solution. Maybe edit this response to include actual code to seal the deal? — Sep 25 '08 at 16:06
Would it be theft if I incorporated this into my answer, to give a single complete criticism? You deserve the rep points, though, for the most advanced answer, so feel free to turn yours into the all-singing all-dancing. — Sep 25 '08 at 16:16
Looks like this example has a problem where the access to lockTable isn't synchronized, which basically can have at least two bad consequences: (1) you might end up performing multiple concurrent operations on StaticCache for the same key, (2) different states of lockTable seen by different CPUs. — Sep 25 '08 at 16:41

score 9 · Accepted Answer · 2008-12-03 13:34:13Z

Strings are not good candidates for synchronization. If you must synchronize on a String ID, it can be done by using the string to create a mutex (see "synchronizing on an ID"). Whether the cost of that algorithm is worth it depends on whether invoking your service involves any significant I/O.

Also:

I hope the StaticCache.get() and set() methods are threadsafe.

String.intern() comes at a cost (one that varies between VM implementations) and should be used with care.

MBCookMBCook 12.4k63340 · Accepted Answer · 2008-09-25 16:04:40Z

Others have suggested interning the strings, and that will work.

The problem is that Java has to keep interned strings around. I was told it does this even if you're not holding a reference because the value needs to be the same the next time someone uses that string. This means interning all the strings may start eating up memory, which with the load you're describing could be a big problem.

I have seen two solutions to this:

You could synchronize on another object

Instead of the email, make an object that holds the email (say the User object) that holds the value of email as a variable. If you already have another object that represents the person (say you already pulled something from the DB based on their email) you could use that. By implementing the equals method and the hashcode method you can make sure Java considers the objects the same when you do a static cache.contains() to find out if the data is already in the cache (you'll have to synchronize on the cache).

Actually, you could keep a second Map for objects to lock on. Something like this:

Map<String, Object> emailLocks = new HashMap<String, Object>();



Object lock = null;



synchronized (emailLocks) {

    lock = emailLocks.get(emailAddress);



    if (lock == null) {

        lock = new Object();

        emailLocks.put(emailAddress, lock);

    }

}



synchronized (lock) {

    // See if this email is in the cache

    // If so, serve that

    // If not, generate the data



    // Since each of this person's threads synchronizes on this, they won't run

    // over eachother. Since this lock is only for this person, it won't effect

    // other people. The other synchronized block (on emailLocks) is small enough

    // it shouldn't cause a performance problem.

}

This will prevent 15 fetches on the same email address at one. You'll need something to prevent too many entries from ending up in the emailLocks map. Using LRUMaps from Apache Commons would do it.

This will need some tweaking, but it may solve your problem.

Use a different key

If you are willing to put up with possible errors (I don't know how important this is) you could use the hashcode of the String as the key. ints don't need to be interned.

Summary

I hope this helps. Threading is fun, isn't it? You could also use the session to set a value meaning "I'm already working on finding this" and check that to see if the second (third, Nth) thread needs to attempt to create the or just wait for the result to show up in the cache. I guess I had three suggestions.

Your emailLocks leaks - this approach needs some sort of reference count and release mechanism. — Sep 25 '08 at 16:21
"ints don't need to be interned" - but don't have monitors. Your summary hints at the exciting world of wait/notifyAll, which is always a laugh :-) — Sep 25 '08 at 16:24
@McDowell: I think a WeakHashMap might actually be appropriate here. However, that's such a rare occurrence than I'm hesitant to say any more... — Sep 25 '08 at 16:32
@McDowell: That's why I said the LRUMap in the comments, it would prevent you from ever having more than say 30 entries. You could also setup a thread to do it. — Sep 25 '08 at 18:19
@onebyone: Good point. Integers do, but an Integer doesn't equal an Integer like int, you have to use .intValue() or .equals(). LIke I said, they were quick suggestions. I would have caught that when I noticed the code didn't work :) — Sep 25 '08 at 18:20

oxbow_lakesoxbow_lakes 108k48291433 · Accepted Answer · 2008-09-27 17:26:01Z

You can use the 1.5 concurrency utilities to provide a cache designed to allow multiple concurrent access, and a single point of addition (i.e. only one thread ever performing the expensive object "creation"):

 private ConcurrentMap<String, Future<SomeData> cache;

 private SomeData getSomeDataByEmail(final WebServiceInterface service, final String email) throws Exception {



  final String key = "Data-" + email;

  Callable<SomeData> call = new Callable<SomeData>() {

      public SomeData call() {

          return service.getSomeDataForEmail(email);

      }

  }

  FutureTask<SomeData> ft; ;

  Future<SomeData> f = cache.putIfAbsent(key, ft= new FutureTask<SomeData>(call)); //atomic

  if (f == null) { //this means that the cache had no mapping for the key

      f = ft;

      ft.run();

  }

  return f.get(); //wait on the result being available if it is being calculated in another thread

}

Obviously, this doesn't handle exceptions as you'd want to, and the cache doesn't have eviction built in. Perhaps you could use it as a basis to change your StaticCache class, though.

Liking this solution a lot - no need to keep a separate in-progress sentinel or separate map of keys to locks - it either contains the Future (another thread is working on it, just call get), or it doesn't (no other threads working on it, do the expensive calculation) — Nov 2 '16 at 1:17

kohlermkohlerm 2,25811417 · Accepted Answer · 2008-10-08 08:48:10Z

Use a decent caching framework such as ehcache.

Implementing a good cache is not as easy as some people believe.

Regarding the comment that String.intern() is a source of memory leaks, that is actually not true.
Interned Strings are garbage collected,it just might take longer because on certain JVM'S (SUN) they are stored in Perm space which is only touched by full GC's.

score 3 · Accepted Answer · 2019-01-01 22:58:38Z

Here is a safe short Java 8 solution that uses a map of dedicated lock objects for synchronization:

private static final Map<String, Object> keyLocks = new ConcurrentHashMap<>();



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {

        SomeData data = StaticCache.get(key);

        if (data == null) {

            data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data);

        }

    }

    return data;

}

It has a drawback that keys and lock objects would retain in map forever.

This can be worked around like this:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {

        try {

            SomeData data = StaticCache.get(key);

            if (data == null) {

                data = service.getSomeDataForEmail(email);

                StaticCache.set(key, data);

            }

        } finally {

            keyLocks.remove(key); // vulnerable to race-conditions

        }

    }

    return data;

}

But then popular keys would be constantly reinserted in map with lock objects being reallocated.

Update: And this leaves race condition possibility when two threads would concurrently enter synchronized section for the same key but with different locks.

So it may be more safe and efficient to use expiring Guava Cache:

private static final LoadingCache<String, Object> keyLocks = CacheBuilder.newBuilder()

        .expireAfterAccess(10, TimeUnit.MINUTES) // max lock time ever expected

        .build(CacheLoader.from(Object::new));



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    final String key = "Data-" + email;

    synchronized (keyLocks.getUnchecked(key)) {

        SomeData data = StaticCache.get(key);

        if (data == null) {

            data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data);

        }

    }

    return data;

}

Note that it's assumed here that StaticCache is thread-safe and wouldn't suffer from concurrent reads and writes for different keys.

You could in theory use the calculated key as lock-object, then you avoid re-allocations of new objects (the key needs to be calculated either way). If a ConcurrentHashMap is used for keyLocks that shouldn't leave room for race conditions? computeIfAbsent would only insert the first one, later attempts would return the identical instance? — Dec 30 '18 at 10:16
@knittl, yes, but it would not be reliable without intern(). And it would suffer of all downsides mentioned in Martin Probst's answer here. — Dec 30 '18 at 17:35
why would it not be reliable without interning the string? As far as I see all synchronize block would use the same string instance (because it is put in the map and reused). The string is local, because it is concatenated inside the method. — Dec 31 '18 at 10:22
@knittl, there is no guarantee that subsequent concatenations would return the same object. And there is no guarantee that concatenated string would not be the same as interned instance that may be synchronized in some unrelated place elsewhere. — Jan 1 at 21:18
You don't need identical objects. You are using the concatenated strings as key in the hashmap. They provide hash and value equality, so you will find the correct value for equal keys. But having strings auto-interned is a good point! (I doubt it though, because the emitted byte code is usually a regular string builder (or fancy invokedynamic code in newer JDK versions)) — Jan 1 at 21:43

AlexanderAlexander 7,20321921 · Accepted Answer · 2008-09-25 15:39:28Z

2

Your main problem is not just that there might be multiple instances of String with the same value. The main problem is that you need to have only one monitor on which to synchronize for accessing the StaticCache object. Otherwise multiple threads might end up concurrently modifying StaticCache (albeit under different keys), which most likely doesn't support concurrent modification.

answered Sep 25 '08 at 15:39

Alexander

7,20321921

That's solved by making it use, for example, ConcurrentHashmap.

– Steve Jessop
Sep 25 '08 at 16:08

You're right, I forgot about ConcurrentHashmap! However, judging by the update of the question, StaticCache doesn't really support concurrent modification.

– Alexander
Sep 25 '08 at 16:37

add a comment |

score 2 · Accepted Answer · 2008-09-25 15:46:21Z

The call:

   final String key = "Data-" + email;

creates a new object every time the method is called. Because that object is what you use to lock, and every call to this method creates a new object, then you are not really synchronizing access to the map based on the key.

This further explain your edit. When you have a static string, then it will work.

Using intern() solves the problem, because it returns the string from an internal pool kept by the String class, that ensures that if two strings are equal, the one in the pool will be used. See

http://java.sun.com/j2se/1.4.2/docs/api/java/lang/String.html#intern()

igor.zhigor.zh 626710 · Accepted Answer · 2018-05-11 18:31:56Z

This question seems to me a bit too broad, and therefore it instigated equally broad set of answers. So I'll try to answer the question I have been redirected from, unfortunately that one has been closed as duplicate.

public class ValueLock<T> {



    private Lock lock = new ReentrantLock();

    private Map<T, Condition> conditions  = new HashMap<T, Condition>();



    public void lock(T t){

        lock.lock();

        try {

            while (conditions.containsKey(t)){

                conditions.get(t).awaitUninterruptibly();

            }

            conditions.put(t, lock.newCondition());

        } finally {

            lock.unlock();

        }

    }



    public void unlock(T t){

        lock.lock();

        try {

            Condition condition = conditions.get(t);

            if (condition == null)

                throw new IllegalStateException();// possibly an attempt to release what wasn't acquired

            conditions.remove(t);

            condition.signalAll();

        } finally {

            lock.unlock();

        }

    }

Upon the (outer) lock operation the (inner) lock is acquired to get an exclusive access to the map for a short time, and if the correspondent object is already in the map, the current thread will wait,
otherwise it will put new Condition to the map, release the (inner) lock and proceed,
and the (outer) lock is considered obtained.
The (outer) unlock operation, first acquiring an (inner) lock, will signal on Condition and then remove the object from the map.

The class does not use concurrent version of Map, because every access to it is guarded by single (inner) lock.

Please notice, the semantic of lock() method of this class is different that of ReentrantLock.lock(), the repeated lock() invocations without paired unlock() will hang current thread indefinitely.

An example of usage that might be applicable to the situation, the OP described

    ValueLock<String> lock = new ValueLock<String>();

    // ... share the lock   

    String email = "...";

    try {

        lock.lock(email);

        //... 

    } finally {

        lock.unlock(email);

    }

I'm very interested by your ValueLock object, but confused by it. The way I read it, if Thread 2 calls` lock()` while Thread1 is already doing something, it will wait at the conditions.awaitUninteruptibly(). Which will block T1 from calling the unlock() method since the ReentrantLock is being held by T2 at that point. Am I mis-reading something? — Nov 27 '18 at 23:22
@Eric B, Neither of Threads 1 or 2 of yours holds an inner (Reentrant) lock except for very short period of time while operating Value-to-Condition map. When Thread 1 holds an Value lock it means it holds a lock on certain Value-bound Condition, not on inner (Reentrant) Lock. When Thread 2 awaitsUninterruptibly on this Value-bound Condition it actually releases the inner(Reentrant) Lock. Until it acquires this Condition, then acquires an inner (Reentrant) Lock, puts the Condition into map and immediately releases inner (Reentrant) Lock. Please let me know if I will have to rephrase it — Nov 29 '18 at 19:28

smerny 13.2k185069 · Accepted Answer · 2013-08-22 19:34:17Z

This is rather late, but there is quite a lot of incorrect code presented here.

In this example:

private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {





  SomeData data = null;

  final String key = "Data-" + email;



  synchronized(key) {      

    data =(SomeData) StaticCache.get(key);



    if (data == null) {

        data = service.getSomeDataForEmail(email);

        StaticCache.set(key, data, CACHE_TIME);

    }

    else {

      logger.debug("getSomeDataForEmail: using cached object");

    }

  }



  return data;

}

The synchronization is incorrectly scoped. For a static cache that supports a get/put API, there should be at least synchronization around the get and getIfAbsentPut type operations, for safe access to the cache. The scope of synchronization will be the cache itself.

If updates must be made to the data elements themselves, that adds an additional layer of synchronization, which should be on the individual data elements.

SynchronizedMap can be used in place of explicit synchronization, but care must still be observed. If the wrong APIs are used (get and put instead of putIfAbsent) then the operations won't have the necessary synchronization, despite the use of the synchronized map. Notice the complications introduced by the use of putIfAbsent: Either, the put value must be computed even in cases when it is not needed (because the put cannot know if the put value is needed until the cache contents are examined), or requires a careful use of delegation (say, using Future, which works, but is somewhat of a mismatch; see below), where the put value is obtained on demand if needed.

The use of Futures is possible, but seems rather awkward, and perhaps a bit of overengineering. The Future API is at it's core for asynchronous operations, in particular, for operations which may not complete immediately. Involving Future very probably adds a layer of thread creation -- extra probably unnecessary complications.

The main problem of using Future for this type of operation is that Future inherently ties in multi-threading. Use of Future when a new thread is not necessary means ignoring a lot of the machinery of Future, making it an overly heavy API for this use.

MattW.MattW. 10.2k44660 · Accepted Answer · 2008-09-25 15:59:34Z

0

Why not just render a static html page that gets served to the user and regenerated every x minutes?

answered Sep 25 '08 at 15:59

MattW.

10.2k44660

add a comment |

John GardnerJohn Gardner 19.5k34163 · Accepted Answer · 2008-09-25 19:06:59Z

I'd also suggest getting rid of the string concatenation entirely if you don't need it.

final String key = "Data-" + email;

Is there other things/types of objects in the cache that use the email address that you need that extra "Data-" at the beginning of the key?

if not, i'd just make that

final String key = email;

and you avoid all that extra string creation too.

celencelen 11 · Accepted Answer · 2016-10-12 02:33:27Z

0

other way synchronizing on string object :

String cacheKey = ...;



    Object obj = cache.get(cacheKey)



    if(obj==null){

    synchronized (Integer.valueOf(Math.abs(cacheKey.hashCode()) % 127)){

          obj = cache.get(cacheKey)

         if(obj==null){

             //some cal obtain obj value,and put into cache

        }

    }

}

answered Oct 12 '16 at 2:33

celen

11

This has the same drawbacks as String.intern with higher probability of getting into trouble.

– Vadzim
Nov 11 '17 at 21:41

add a comment |

score 0 · Accepted Answer · 2017-11-26 20:25:46Z

In case others have a similar problem, the following code works, as far as I can tell:

import java.util.Map;

import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.atomic.AtomicInteger;

import java.util.function.Supplier;



public class KeySynchronizer<T> {



    private Map<T, CounterLock> locks = new ConcurrentHashMap<>();



    public <U> U synchronize(T key, Supplier<U> supplier) {

        CounterLock lock = locks.compute(key, (k, v) -> 

                v == null ? new CounterLock() : v.increment());

        synchronized (lock) {

            try {

                return supplier.get();

            } finally {

                if (lock.decrement() == 0) {

                    // Only removes if key still points to the same value,

                    // to avoid issue described below.

                    locks.remove(key, lock);

                }

            }

        }

    }



    private static final class CounterLock {



        private AtomicInteger remaining = new AtomicInteger(1);



        private CounterLock increment() {

            // Returning a new CounterLock object if remaining = 0 to ensure that

            // the lock is not removed in step 5 of the following execution sequence:

            // 1) Thread 1 obtains a new CounterLock object from locks.compute (after evaluating "v == null" to true)

            // 2) Thread 2 evaluates "v == null" to false in locks.compute

            // 3) Thread 1 calls lock.decrement() which sets remaining = 0

            // 4) Thread 2 calls v.increment() in locks.compute

            // 5) Thread 1 calls locks.remove(key, lock)

            return remaining.getAndIncrement() == 0 ? new CounterLock() : this;

        }



        private int decrement() {

            return remaining.decrementAndGet();

        }

    }

}

In the case of the OP, it would be used like this:

private KeySynchronizer<String> keySynchronizer = new KeySynchronizer<>();



private SomeData getSomeDataByEmail(WebServiceInterface service, String email) {

    String key = "Data-" + email;

    return keySynchronizer.synchronize(key, () -> {

        SomeData existing = (SomeData) StaticCache.get(key);

        if (existing == null) {

            SomeData data = service.getSomeDataForEmail(email);

            StaticCache.set(key, data, CACHE_TIME);

            return data;

        }

        logger.debug("getSomeDataForEmail: using cached object");

        return existing;

    });

}

If nothing should be returned from the synchronized code, the synchronize method can be written like this:

public void synchronize(T key, Runnable runnable) {

    CounterLock lock = locks.compute(key, (k, v) -> 

            v == null ? new CounterLock() : v.increment());

    synchronized (lock) {

        try {

            runnable.run();

        } finally {

            if (lock.decrement() == 0) {

                // Only removes if key still points to the same value,

                // to avoid issue described below.

                locks.remove(key, lock);

            }

        }

    }

}

score 0 · Accepted Answer · 2018-05-27 13:33:44Z

I've added a small lock class that can lock/synchronize on any key, including strings.

See implementation for Java 8, Java 6 and a small test.

Java 8:

public class DynamicKeyLock<T> implements Lock

{

    private final static ConcurrentHashMap<Object, LockAndCounter> locksMap = new ConcurrentHashMap<>();



    private final T key;



    public DynamicKeyLock(T lockKey)

    {

        this.key = lockKey;

    }



    private static class LockAndCounter

    {

        private final Lock lock = new ReentrantLock();

        private final AtomicInteger counter = new AtomicInteger(0);

    }



    private LockAndCounter getLock()

    {

        return locksMap.compute(key, (key, lockAndCounterInner) ->

        {

            if (lockAndCounterInner == null) {

                lockAndCounterInner = new LockAndCounter();

            }

            lockAndCounterInner.counter.incrementAndGet();

            return lockAndCounterInner;

        });

    }



    private void cleanupLock(LockAndCounter lockAndCounterOuter)

    {

        if (lockAndCounterOuter.counter.decrementAndGet() == 0)

        {

            locksMap.compute(key, (key, lockAndCounterInner) ->

            {

                if (lockAndCounterInner == null || lockAndCounterInner.counter.get() == 0) {

                    return null;

                }

                return lockAndCounterInner;

            });

        }

    }



    @Override

    public void lock()

    {

        LockAndCounter lockAndCounter = getLock();



        lockAndCounter.lock.lock();

    }



    @Override

    public void unlock()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);

        lockAndCounter.lock.unlock();



        cleanupLock(lockAndCounter);

    }





    @Override

    public void lockInterruptibly() throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        try

        {

            lockAndCounter.lock.lockInterruptibly();

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }

    }



    @Override

    public boolean tryLock()

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired = lockAndCounter.lock.tryLock();



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired;

        try

        {

            acquired = lockAndCounter.lock.tryLock(time, unit);

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public Condition newCondition()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);



        return lockAndCounter.lock.newCondition();

    }

}

Java 6:

public class DynamicKeyLock implements Lock
{
private final static ConcurrentHashMap locksMap = new ConcurrentHashMap();
private final T key;

    public DynamicKeyLock(T lockKey) {

        this.key = lockKey;

    }



    private static class LockAndCounter {

        private final Lock lock = new ReentrantLock();

        private final AtomicInteger counter = new AtomicInteger(0);

    }



    private LockAndCounter getLock()

    {

        while (true) // Try to init lock

        {

            LockAndCounter lockAndCounter = locksMap.get(key);



            if (lockAndCounter == null)

            {

                LockAndCounter newLock = new LockAndCounter();

                lockAndCounter = locksMap.putIfAbsent(key, newLock);



                if (lockAndCounter == null)

                {

                    lockAndCounter = newLock;

                }

            }



            lockAndCounter.counter.incrementAndGet();



            synchronized (lockAndCounter)

            {

                LockAndCounter lastLockAndCounter = locksMap.get(key);

                if (lockAndCounter == lastLockAndCounter)

                {

                    return lockAndCounter;

                }

                // else some other thread beat us to it, thus try again.

            }

        }

    }



    private void cleanupLock(LockAndCounter lockAndCounter)

    {

        if (lockAndCounter.counter.decrementAndGet() == 0)

        {

            synchronized (lockAndCounter)

            {

                if (lockAndCounter.counter.get() == 0)

                {

                    locksMap.remove(key);

                }

            }

        }

    }



    @Override

    public void lock()

    {

        LockAndCounter lockAndCounter = getLock();



        lockAndCounter.lock.lock();

    }



    @Override

    public void unlock()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);

        lockAndCounter.lock.unlock();



        cleanupLock(lockAndCounter);

    }





    @Override

    public void lockInterruptibly() throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        try

        {

            lockAndCounter.lock.lockInterruptibly();

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }

    }



    @Override

    public boolean tryLock()

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired = lockAndCounter.lock.tryLock();



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException

    {

        LockAndCounter lockAndCounter = getLock();



        boolean acquired;

        try

        {

            acquired = lockAndCounter.lock.tryLock(time, unit);

        }

        catch (InterruptedException e)

        {

            cleanupLock(lockAndCounter);

            throw e;

        }



        if (!acquired)

        {

            cleanupLock(lockAndCounter);

        }



        return acquired;

    }



    @Override

    public Condition newCondition()

    {

        LockAndCounter lockAndCounter = locksMap.get(key);



        return lockAndCounter.lock.newCondition();

    }

}

Test:

public class DynamicKeyLockTest

{

    @Test

    public void testDifferentKeysDontLock() throws InterruptedException

    {

        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(new Object());

        lock.lock();

        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);

        try

        {

            new Thread(() ->

            {

                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(new Object());

                anotherLock.lock();

                try

                {

                    anotherThreadWasExecuted.set(true);

                }

                finally

                {

                    anotherLock.unlock();

                }

            }).start();

            Thread.sleep(100);

        }

        finally

        {

            Assert.assertTrue(anotherThreadWasExecuted.get());

            lock.unlock();

        }

    }



    @Test

    public void testSameKeysLock() throws InterruptedException

    {

        Object key = new Object();

        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(key);

        lock.lock();

        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);

        try

        {

            new Thread(() ->

            {

                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(key);

                anotherLock.lock();

                try

                {

                    anotherThreadWasExecuted.set(true);

                }

                finally

                {

                    anotherLock.unlock();

                }

            }).start();

            Thread.sleep(100);

        }

        finally

        {

            Assert.assertFalse(anotherThreadWasExecuted.get());

            lock.unlock();

        }

    }

}

marc_s 581k13011221268 · Accepted Answer · 2019-03-08 21:49:39Z

You can safely use String.intern for synchronize if you can reasonably guarantee that the string value is unique across your system. UUIDS are a good way to approach this. You can associate a UUID with your actual string key, either via a cache, a map, or maybe even store the uuid as a field on your entity object.

    @Service   

    public class MySyncService{



      public Map<String, String> lockMap=new HashMap<String, String>();



      public void syncMethod(String email) {



        String lock = lockMap.get(email);

        if(lock==null) {

            lock = UUID.randomUUID().toString();

            lockMap.put(email, lock);

        }   



        synchronized(lock.intern()) {

                //do your sync code here

        }

    }

Search This Blog

Ufyukyu

Synchronizing on String objects in Java

17 Answers
17

Your Answer

Post as a guest

17 Answers
17

17 Answers
17

Post as a guest

Popular posts from this blog

MongoDB - Not Authorized To Execute Command

How to fix TextFormField cause rebuild widget in Flutter

Npm cannot find a required file even through it is in the searched directory

Category

Random preview

Synchronizing on String objects in Java

17 Answers 17

Your Answer

Sign up or log in

Post as a guest

Post as a guest

17 Answers 17

17 Answers 17

Sign up or log in

Post as a guest

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Popular posts from this blog

MongoDB - Not Authorized To Execute Command

How to fix TextFormField cause rebuild widget in Flutter

Npm cannot find a required file even through it is in the searched directory

17 Answers
17

17 Answers
17

17 Answers
17