Ways to improve upon that code using only JDK (6) provided classes? (concurrency, thread safety)

Question

(preliminary note: maybe this is better suited for codereview?)

EDIT Answer to self; I believe this answer covers all my needs/problems, but of course, comments are welcome. Original question left below for reference.

Hello,

Of interest here is the .getSources() method. This method is meant to return a list of message sources for a given Locale.

The two central data structures to this method are sources and failedLookups, see the code for comments.

This particular implementation of .getSources() can only ever return either an empty list or a single-element list, depending on the tryAndLookup() method which prototype is:

protected abstract MessageSource tryAndLookup(final Locale locale)
    throws IOException;

Right now, the logic of the code is as follows:

• if the message source for that locale has already been successfully looked up, it is returned;
• from this point on, no lookup has been done; it is however unknown whether this means that a previous lookup attempt has been done: check the set of failed lookups, if the locale to lookup is in there, it is a known failure, return the empty list;
• now, the known situation is that this locale lookup has actually never been performed: do perform it; depending on the result of the tryAndLookup method, record either a success or a failure.

Now, why I go to such lengths: I have no control over tryAndLookup(); it may take an inordinate amount of time to execute before returning a valid source or failing. As a result, I am reluctant to using a coarse lock or synchronized block.

/**
 * Set of locales known to have failed lookup.
 *
 * <p>When a locale is in this set, it will not attempt to be reloaded.</p>
 */
private final Set<Locale> lookupFailures
    = new CopyOnWriteArraySet<Locale>();

/**
 * Set of message sources successfully looked up
 *
 * <p>When a source is in there, it is there permanently for now.</p>
 */
private final ConcurrentMap<Locale, MessageSource> sources
    = new ConcurrentHashMap<Locale, MessageSource>();

@Override
protected final List<MessageSource> getSources(final Locale locale)
{
    MessageSource source = sources.get(locale);

    /*
     * If found, return it
     */
    if (source != null)
        return Arrays.asList(source);

    /*
     * If it is a registered failure, return the empty list
     */
    if (lookupFailures.contains(locale))
        return Collections.emptyList();

    /*
     * OK, try and look it up. On success, register it in the sources map.
     * On failure, record the failure an return the empty list.
     */
    try {
        source = tryAndLookup(locale);
        sources.putIfAbsent(locale, source);
        // EDIT: fix for bug pinpointed by JBNizet
        // was:
        // return Arrays.asList(source);
        // now is:
        return Arrays.asList(sources.get(locale));
    } catch (IOException ignored) {
        lookupFailures.add(locale);
        return Collections.emptyList();
    }
}

My question here is threefold:

• I purposefully limit myself to classes only available to the JDK; I chose ConcurrentHashMap as a ConcurrentMap implementation, and CopyOnWriteArraySet as a thread-safe Set implementation; from the javadoc, these are the best I could find. But was I misled somewhere?
• I think this code is eventually thread safe; some corner cases may lead to lookups being done more than once for instance, but then this is why I do .putIfAbsent(); right now I have always used, and trusted, Guava's LoadingCache for caching purposes, and this is my first foray out of this territory; is this code actually thread safe?
• this code has a fatal defect: more than one thread may be executing tryAndLookup() at the same time... What solutions exist to have this method executed only once per lookup?

Answer 1

As an alternative to the CopyOnWriteArraySet, you could use a ConcurrentHashMap with meaningless values (e.g. always use Boolean.TRUE as the value - you only care about the keys), or else you could use a ConcurrentSkipListSet which is essentially a concurrent TreeSet that uses a skiplist instead of a balanced binary tree.

Assuming that tryAndLookup is fast and doesn't have any side-effects, it doesn't really matter if you occasionally execute it more than once, as as such your "eventually thread-safe" code is thread-safe in the sense that it won't produce any anomalous behavior. (If it's slow then it may be more efficient to use locks in order to ensure that you execute it as few times as possible, but in this case your code would still be free from anomalous behavior. If it has side-effects then you may have a data race if you execute it twice on the same locale.)

Edit Because tryAndLookup may have side-effects, you can either synchronize on locale, or else you can modify your method as follows

private final MessageSource nullSource = new MessageSource(); // Used in place of null in the ConcurrentHashMap, which does not accept null keys or values

protected final List<MessageSource> getSources(final Locale locale) {
...

    try {
        if(sources.putIfAbsent(locale, nullSource) == null) {
            source = tryAndLookup(locale);
            sources.replace(locale, nullSource, source);
            return Arrays.asList(sources.get(locale));
        } else {
            // Another thread is calling tryAndLookup, so wait for it to finish
            while(sources.get(locale) == nullSource && !lookupFailures.contains(locale))
                Thread.sleep(500);
            }
            if(sources.get(locale) != nullSource) {
                return Arrays.asList(sources.get(locale));
            } else {
                return Collections.emptyList();
            }
        }
    } catch (IOException ignored) {
        lookupFailures.add(locale);
        return Collections.emptyList();
    }
}

Answer 2

You can run the first two steps in a synchronized block using simpler data types. If in those steps you find you need to perform the tryAndLookup(), store a Future for the pending result in a separate list of pending lookups before leaving the synchronized block.

Then outside the synchronized block, do the actual lookup. Threads that find they need the same result will find the Future and can get() its result outside the synchronized block.