Custom, simple cache with load timeout and expiry; thread safety, tests

The following is a code from one of my projects which implements a loading cache with timeout and expiry. The associated tests are here (mixed with a good number of argument sanity checks tests). I seek comments on the thread safety aspects in particular. I think it is, but I may miss something.

Also, do you see ways it could be improved?

```
/**
* A caching, on-demand loading message source provider with configurable expiry
*
* This class uses a {@link MessageSourceLoader} internally to look up
* message sources. As is the case for {@link StaticMessageSourceProvider}, you
* can also set a default source if the loader fails to grab a source.
*
* Apart from the loader, you can customize two aspects of the provider:
*
*
* its load timeout (5 seconds by default);
* its expiry time (10 minutes by default).
*
*
* Note that the expiry time is periodic only, and not per source. The
* loading result (success or failure) is recorded permanently until the expiry
* time kicks in, except when the timeout kicks in; in this case, loading
* will be retried.
*
* You cannot instantiate that class directly; use {@link #newBuilder()} to
* obtain a builder class and set up your provider.
*/
@ThreadSafe
public final class LoadingMessageSourceProvider
implements MessageSourceProvider
{
/*
* Use daemon threads. We don't give control to the user about the
* ExecutorService, and we don't have a reliable way to shut it down (a JVM
* shutdown hook does not get involved on a webapp shutdown, so we cannot
* use that...).
*/
private static final ThreadFactory THREAD_FACTORY = new ThreadFactory()
{
private final ThreadFactory factory = Executors.defaultThreadFactory();

@Override
public Thread newThread(final Runnable r)
{
final Thread ret = factory.newThread(r);
ret.setDaemon(true);
return ret;
}
};

private static final InternalBundle BUND

I think you do have a race condition. The test below should expose it.

@Mock
private MessageSource messageSource;

@Test
public void testExpiryDuringLoadingDoesNotInterfereWithGetMessageSourceCallInProgress() throws Exception {

    MessageSourceProvider sourceProvider = LoadingMessageSourceProvider.newBuilder().setExpiryTime(20, TimeUnit.MILLISECONDS)
            .setDefaultSource(messageSource).setLoader(new SlowLoader())
            .build();
    MessageSource messageSource = sourceProvider.getMessageSource(Locale.CHINA);
    assertThat(messageSource).isEqualTo(messageSource);
}

private class SlowLoader implements MessageSourceLoader {
    @Override
    public MessageSource load(Locale locale) {
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        }
        return messageSource;
    }
}

While I force the race condition to fail in the test by making the expiry time really short, and the loading time long, that is in itself not a necessary condition.

Here's the code relevant snippet with added comments :

synchronized (sources) {                // cannot clean up here
    task = sources.get(locale);
    if (task == null || task.isCancelled()) {
        task = loadingTask(locale);
        sources.put(locale, task);
        service.execute(task);
    }
}
                                        // yet here it can : task cancellation may happen during cleanup here
                                        // expiry may have been imminent just before the resource was requested
try {
    final MessageSource source = task.get(timeoutDuration, timeoutUnit); // load may even be short : cancelation could have happened even before this request.
    return source == null ? defaultSource : source;

StackExchange Code Review Q#27452, answer score: 3