java - 仅使用 JDK (6) 提供的类来改进该代码的方法？（并发，线程安全）

Question

（初步说明：也许这更适合代码审查？）

编辑对自己的回答；我相信这个答案涵盖了我所有的需求/问题，但当然，欢迎发表评论。原始问题留在下面以供参考。

你好，

这里有趣的是.getSources()方法。此方法旨在返回给定的消息源列表Locale。

此方法的两个中心数据结构是sources和failedLookups，请参阅代码以获取注释。

这个特定的实现.getSources()只能返回一个空列表或一个单元素列表，这取决于tryAndLookup()原型的方法：

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

现在，代码的逻辑如下：

如果该语言环境的消息源已被成功查找，则返回；
从此时起，没有进行任何查找；然而，这是否意味着先前的查找尝试已经完成是未知的：检查失败的查找集合，如果要查找的语言环境在那里，这是一个已知的失败，返回空列表；
现在，已知的情况是这个语言环境查找实际上从未执行过：执行它；根据tryAndLookup方法的结果，记录成功或失败。

现在，我为什么要这么费劲：我无法控制tryAndLookup()；在返回有效源或失败之前执行可能需要过多的时间。因此，我不愿意使用粗锁或synchronized阻塞。

/**
 * 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();
    }
}

我的问题有三个：

我故意将自己限制在只对 JDK 可用的类；我选择了ConcurrentHashMap作为一个ConcurrentMap实现，并且CopyOnWriteArraySet作为一个线程安全的Set实现；从javadoc，这些是我能找到的最好的。但我是不是在什么地方被误导了？
我认为这段代码最终是线程安全的；例如，某些极端情况可能会导致多次查找，但这就是我这样做的原因.putIfAbsent()；现在我一直使用并信任 GuavaLoadingCache用于缓存目的，这是我第一次走出这个领域；这段代码实际上是线程安全的吗？
这段代码有一个致命缺陷：可能同时执行多个线程tryAndLookup()... 有什么解决方案可以让这个方法每次查找只执行一次？

score 1 · Accepted Answer

作为的替代方案CopyOnWriteArraySet，您可以使用ConcurrentHashMap具有无意义值的 a （例如，始终Boolean.TRUE用作值 - 您只关心键），或者您可以使用ConcurrentSkipListSet，它本质上是一个并发的TreeSet，它使用跳过列表而不是平衡的二进制树。

假设这tryAndLookup很快并且没有任何副作用，那么您是否偶尔多次执行它并不重要，因为您的“最终线程安全”代码在它赢得的意义上是线程安全的不会产生任何异常行为。（如果它很慢，那么使用锁可能会更有效，以确保您尽可能少地执行它，但在这种情况下，您的代码仍然不会出现异常行为。如果它有副作用，那么您可以如果您在同一语言环境中执行两次，则会出现数据竞争。）

编辑因为tryAndLookup可能有副作用，你可以同步locale，或者你可以修改你的方法如下

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

score 1 · Accepted Answer

您可以使用更简单的数据类型在同步块中运行前两个步骤。如果在这些步骤中您发现需要执行，则在离开同步块之前将未决结果的FuturetryAndLookup()存储在单独的未决查找列表中。

然后在同步块之外，进行实际查找。发现它们需要相同结果的线程将找到 Future 并且可以get()在同步块之外找到它的结果。

score 0 · Accepted Answer

回答自己...

该算法已完全修改。它基于 JDK 的FutureTask，因为它有两个非常好的属性：

它的.run()方法是异步的；
它是有状态的，无论成功还是失败——意思是，它将返回已经计算的结果或抛出的异常（包装在中ExecutionException）.get()。

这对使用的数据结构有很大的影响：

先前存在的lookupFailures记录失败的集合已经消失；
现有的地图，原来是 a ConcurrentMap，现在被 a plain 取代，由 aMap守卫ReentrantLock；它的值现在FutureTask<MessageSource>不是MessageSource.

这对算法也有很大的影响，算法要简单得多：

锁定地图；
查找语言环境的任务；如果它以前不存在，创建它，然后.run()它；
解锁地图；
.get()结果：成功时返回单个元素列表，失败时返回空列表。

完整代码，带注释：

@ThreadSafe
public abstract class CachedI18NMessageBundle
    extends I18NMessageBundle
{
    /**
     * Map pairing locales with {@link FutureTask} instances returning message
     * sources
     *
     * <p>There will only ever be one task associated with one locale; we
     * therefore choose to make it a normal map, guarded by a {@link
     * ReentrantLock}.</p>
     *
     * <p>The tasks' {@link FutureTask#run()} method will be executed the first
     * time this object is initialized.</p>
     */
    @GuardedBy("lock")
    private final Map<Locale, FutureTask<MessageSource>> lookups
        = new HashMap<Locale, FutureTask<MessageSource>>();

    /**
     * Lock used to guarantee exclusive access to the {@link #lookups} map
     */
    private final Lock lock = new ReentrantLock();

    @Override
    protected final List<MessageSource> getSources(final Locale locale)
    {
        FutureTask<MessageSource> task;

        /*
         * Grab an exclusive lock to the lookups map. The lock is held only for
         * the time necessary to grab the FutureTask or create it (and run it)
         * if it didn't exist previously.
         *
         * We can do this, since FutureTask's .run() is asynchronous.
         */
        lock.lock();
        try {
            /*
             * Try and see whether there is already a FutureTask associated with
             * this locale.
             */
            task = lookups.get(locale);
            if (task == null) {
                /*
                 * If not, create it and run it.
                 */
                task = lookupTask(locale);
                lookups.put(locale, task);
                task.run();
            }
        } finally {
            lock.unlock();
        }

        /*
         * Try and get the result for this locale; on any failure event (either
         * an IOException thrown by tryAndLookup() or a thread interrupt),
         * return an empty list.
         */
        try {
            return Arrays.asList(task.get());
        } catch (ExecutionException ignored) {
            return Collections.emptyList();
        } catch (InterruptedException  ignored) {
            return Collections.emptyList();
        }
    }

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

    @Override
    public final Builder modify()
    {
        throw new IllegalStateException("cached bundles cannot be modified");
    }

    /**
     * Wraps an invocation of {@link #tryAndLookup(Locale)} into a {@link
     * FutureTask}
     *
     * @param locale the locale to pass as an argument to {@link
     * #tryAndLookup(Locale)}
     * @return a {@link FutureTask}
     */
    private FutureTask<MessageSource> lookupTask(final Locale locale)
    {
        final Callable<MessageSource> callable = new Callable<MessageSource>()
        {
            @Override
            public MessageSource call()
                throws IOException
            {
                return tryAndLookup(locale);
            }
        };

        return new FutureTask<MessageSource>(callable);
    }
}

我什至可以在查找成功和失败时测试“创建的单个任务”，因为tryAndLookup()它是抽象的，因此可以使用 Mockito 进行“间谍活动”。完整的测试类源在这里（方法onlyOneTaskIsCreatedPer*LocaleLookup()）。

java - 仅使用 JDK (6) 提供的类来改进该代码的方法？（并发，线程安全）

3 回答 3

Related

Reference