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 xpiry 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;

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;

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;
    }
}

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 xpiry 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;