Fix (NonNull)Lazy.Concurrent using a global lock (#7403)

* Fix Lazy.Concurrent using a global lock, thus preventing multiple threads from resolving independent Lazy's at the same time
* Use Lazy with an added null check to implement NonNullLazy
This commit is contained in:
malte0811 2020-10-29 18:09:20 +01:00 committed by GitHub
parent 53eedb0f10
commit 06fe9ccda0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 13 additions and 65 deletions

View file

@ -78,7 +78,7 @@ public interface Lazy<T> extends Supplier<T>
*/ */
final class Concurrent<T> implements Lazy<T> final class Concurrent<T> implements Lazy<T>
{ {
private static final Object lock = new Object(); private volatile Object lock = new Object();
private volatile Supplier<T> supplier; private volatile Supplier<T> supplier;
private volatile T instance; private volatile T instance;
@ -91,14 +91,20 @@ public interface Lazy<T> extends Supplier<T>
@Override @Override
public final T get() public final T get()
{ {
// Copy the lock to a local variable to prevent NPEs if the lock field is set to null between the
// null-check and the synchronization
Object localLock = this.lock;
if (supplier != null) if (supplier != null)
{ {
synchronized (lock) // localLock is not null here because supplier was non-null after we copied the lock and both of them
// are volatile
synchronized (localLock)
{ {
if (supplier != null) if (supplier != null)
{ {
instance = supplier.get(); instance = supplier.get();
supplier = null; supplier = null;
this.lock = null;
} }
} }
} }

View file

@ -20,8 +20,7 @@
package net.minecraftforge.common.util; package net.minecraftforge.common.util;
import javax.annotation.Nonnull; import javax.annotation.Nonnull;
import javax.annotation.Nullable; import java.util.Objects;
import java.util.function.Supplier;
/** /**
* Proxy object for a value that is calculated on first access. * Proxy object for a value that is calculated on first access.
@ -36,7 +35,8 @@ public interface NonNullLazy<T> extends NonNullSupplier<T>
*/ */
static <T> NonNullLazy<T> of(@Nonnull NonNullSupplier<T> supplier) static <T> NonNullLazy<T> of(@Nonnull NonNullSupplier<T> supplier)
{ {
return new NonNullLazy.Fast<>(supplier); Lazy<T> lazy = Lazy.of(supplier::get);
return () -> Objects.requireNonNull(lazy.get());
} }
/** /**
@ -45,65 +45,7 @@ public interface NonNullLazy<T> extends NonNullSupplier<T>
*/ */
static <T> NonNullLazy<T> concurrentOf(@Nonnull NonNullSupplier<T> supplier) static <T> NonNullLazy<T> concurrentOf(@Nonnull NonNullSupplier<T> supplier)
{ {
return new NonNullLazy.Concurrent<>(supplier); Lazy<T> lazy = Lazy.concurrentOf(supplier::get);
} return () -> Objects.requireNonNull(lazy.get());
/**
* Non-thread-safe implementation.
*/
final class Fast<T> implements NonNullLazy<T>
{
private NonNullSupplier<T> supplier;
private T instance;
private Fast(NonNullSupplier<T> supplier)
{
this.supplier = supplier;
}
@Nonnull
@Override
public final T get()
{
if (supplier != null)
{
instance = supplier.get();
supplier = null;
}
return instance;
}
}
/**
* Thread-safe implementation.
*/
final class Concurrent<T> implements NonNullLazy<T>
{
private static final Object lock = new Object();
private volatile NonNullSupplier<T> supplier;
private volatile T instance;
private Concurrent(NonNullSupplier<T> supplier)
{
this.supplier = supplier;
}
@Nonnull
@Override
public final T get()
{
if (supplier != null)
{
synchronized (lock)
{
if (supplier != null)
{
instance = supplier.get();
supplier = null;
}
}
}
return instance;
}
} }
} }