From 4c9dccf996dc062f8f77d7013de504141d9c8306 Mon Sep 17 00:00:00 2001 From: kimtaehun <46879264+Bue-von-hon@users.noreply.github.com> Date: Sat, 3 Jun 2023 17:21:50 +0900 Subject: [PATCH] Replace synchronized under core/common with reentrantLock (#4578) Motivation: for virtual threading, we should replace synchronized with reentrantLock resolves part of #4510 Modifications: replace all synchronized with reentrantLock under `core/common` Result: no more synchronized blocks on `core/common` Co-authored-by: Trustin Lee --- .../common/DefaultConcurrentAttributes.java | 18 +++++-- .../common/DefaultDependencyInjector.java | 51 ++++++++++++------- .../common/logging/DefaultRequestLog.java | 25 ++++++--- .../common/util/ReentrantShortLock.java | 2 +- 4 files changed, 67 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultConcurrentAttributes.java b/core/src/main/java/com/linecorp/armeria/common/DefaultConcurrentAttributes.java index 1dc19acd051..a6f92f3cb7e 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultConcurrentAttributes.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultConcurrentAttributes.java @@ -46,6 +46,7 @@ import com.google.common.collect.Iterators; import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.internal.common.util.ReentrantShortLock; import io.netty.util.AttributeKey; @@ -108,7 +109,8 @@ private T attr(AttributeKey key, boolean ownAttr) { return null; } - synchronized (head) { + head.lock(); + try { DefaultAttribute curr = head; for (;;) { final DefaultAttribute next = curr.next; @@ -126,6 +128,8 @@ private T attr(AttributeKey key, boolean ownAttr) { } curr = next; } + } finally { + head.unlock(); } } @@ -171,7 +175,8 @@ private Object setAttr(AttributeKey key, @Nullable T value, SetAttrMode m head = attributes.get(i); } - synchronized (head) { + head.lock(); + try { DefaultAttribute curr = head; for (;;) { final DefaultAttribute next = curr.next; @@ -190,6 +195,8 @@ private Object setAttr(AttributeKey key, @Nullable T value, SetAttrMode m curr = next; } + } finally { + head.unlock(); } } @@ -231,7 +238,8 @@ public boolean remove(AttributeKey key) { return false; } - synchronized (head) { + head.lock(); + try { DefaultAttribute curr = head; for (;;) { final DefaultAttribute next = curr.next; @@ -245,6 +253,8 @@ public boolean remove(AttributeKey key) { } curr = next; } + } finally { + head.unlock(); } } @@ -368,7 +378,7 @@ private enum SetAttrMode { } @VisibleForTesting - static final class DefaultAttribute implements Entry, T> { + static final class DefaultAttribute extends ReentrantShortLock implements Entry, T> { @Nullable private final AttributeKey key; diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultDependencyInjector.java b/core/src/main/java/com/linecorp/armeria/common/DefaultDependencyInjector.java index 550fa51ca22..b4a4bc36b0f 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultDependencyInjector.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultDependencyInjector.java @@ -19,17 +19,22 @@ import java.util.HashMap; import java.util.Map; +import java.util.concurrent.locks.Lock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.common.base.MoreObjects; +import com.linecorp.armeria.internal.common.util.ReentrantShortLock; + final class DefaultDependencyInjector implements DependencyInjector { private static final Logger logger = LoggerFactory.getLogger(DefaultDependencyInjector.class); private final Map, Object> singletons = new HashMap<>(); + + private final Lock lock = new ReentrantShortLock(); private boolean isShutdown; DefaultDependencyInjector(Iterable singletons) { @@ -41,30 +46,40 @@ final class DefaultDependencyInjector implements DependencyInjector { } @Override - public synchronized T getInstance(Class type) { - if (isShutdown) { - throw new IllegalStateException("Already shut down"); - } - final Object instance = singletons.get(type); - if (instance != null) { - //noinspection unchecked - return (T) instance; + public T getInstance(Class type) { + lock.lock(); + try { + if (isShutdown) { + throw new IllegalStateException("Already shut down"); + } + final Object instance = singletons.get(type); + if (instance != null) { + //noinspection unchecked + return (T) instance; + } + return null; + } finally { + lock.unlock(); } - return null; } @Override - public synchronized void close() { - if (isShutdown) { - return; - } - isShutdown = true; - for (Object instance : singletons.values()) { - if (instance instanceof AutoCloseable) { - close((AutoCloseable) instance); + public void close() { + lock.lock(); + try { + if (isShutdown) { + return; + } + isShutdown = true; + for (Object instance : singletons.values()) { + if (instance instanceof AutoCloseable) { + close((AutoCloseable) instance); + } } + singletons.clear(); + } finally { + lock.unlock(); } - singletons.clear(); } private static void close(AutoCloseable closeable) { diff --git a/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java b/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java index 04aa4db70bd..e5cb8aeb269 100644 --- a/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java +++ b/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java @@ -27,11 +27,13 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; +import java.util.concurrent.locks.Lock; import javax.net.ssl.SSLSession; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.errorprone.annotations.concurrent.GuardedBy; import com.linecorp.armeria.common.HttpData; import com.linecorp.armeria.common.HttpHeaders; @@ -52,6 +54,7 @@ import com.linecorp.armeria.common.util.SystemInfo; import com.linecorp.armeria.common.util.UnmodifiableFuture; import com.linecorp.armeria.internal.common.util.ChannelUtil; +import com.linecorp.armeria.internal.common.util.ReentrantShortLock; import com.linecorp.armeria.internal.common.util.TemporaryThreadLocals; import com.linecorp.armeria.server.HttpResponseException; import com.linecorp.armeria.server.HttpStatusException; @@ -95,7 +98,11 @@ final class DefaultRequestLog implements RequestLog, RequestLogBuilder { * Updated by {@link #deferredFlagsUpdater}. */ private volatile int deferredFlags; + + @GuardedBy("lock") private final List pendingFutures = new ArrayList<>(4); + + private final Lock lock = new ReentrantShortLock(); @Nullable private UnmodifiableFuture partiallyCompletedFuture; @Nullable @@ -342,9 +349,12 @@ private CompletableFuture future(int interestedFla } else { final RequestLogFuture[] satisfiedFutures; final RequestLogFuture newFuture = new RequestLogFuture(interestedFlags); - synchronized (pendingFutures) { + lock.lock(); + try { pendingFutures.add(newFuture); - satisfiedFutures = removeSatisfiedFutures(); + satisfiedFutures = removeSatisfiedFutures(pendingFutures, flags); + } finally { + lock.unlock(); } if (satisfiedFutures != null) { completeSatisfiedFutures(satisfiedFutures, partial(flags)); @@ -386,8 +396,11 @@ private void updateFlags(int flags) { if (flagsUpdater.compareAndSet(this, oldFlags, newFlags)) { final RequestLogFuture[] satisfiedFutures; - synchronized (pendingFutures) { - satisfiedFutures = removeSatisfiedFutures(); + lock.lock(); + try { + satisfiedFutures = removeSatisfiedFutures(pendingFutures, newFlags); + } finally { + lock.unlock(); } if (satisfiedFutures != null) { final RequestLog log = partial(newFlags); @@ -408,12 +421,12 @@ private static void completeSatisfiedFutures(RequestLogFuture[] satisfiedFutures } @Nullable - private RequestLogFuture[] removeSatisfiedFutures() { + private static RequestLogFuture[] removeSatisfiedFutures(List pendingFutures, + int flags) { if (pendingFutures.isEmpty()) { return null; } - final int flags = this.flags; final int maxNumListeners = pendingFutures.size(); final Iterator i = pendingFutures.iterator(); RequestLogFuture[] satisfied = null; diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java b/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java index 28bd8f13de1..626ea48245a 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java @@ -25,7 +25,7 @@ * This lock may be preferred over {@link ReentrantLock} when it is known that the * lock won't block the event loop over long periods of time. */ -public final class ReentrantShortLock extends ReentrantLock { +public class ReentrantShortLock extends ReentrantLock { private static final long serialVersionUID = 8999619612996643502L; @Override