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 <[email protected]>
line · Jun 3, 2023 · 4c9dccf · 4c9dccf
1 parent 61b0d5c
commit 4c9dccf
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 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
@@ -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> T attr(AttributeKey<T> 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> T attr(AttributeKey<T> key, boolean ownAttr) {
                 }
                 curr = next;
             }
+        } finally {
+            head.unlock();
         }
     }
 
@@ -171,7 +175,8 @@ private <T> Object setAttr(AttributeKey<T> 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 <T> Object setAttr(AttributeKey<T> key, @Nullable T value, SetAttrMode m
 
                 curr = next;
             }
+        } finally {
+            head.unlock();
         }
     }
 
@@ -231,7 +238,8 @@ public <T> boolean remove(AttributeKey<T> key) {
             return false;
         }
 
-        synchronized (head) {
+        head.lock();
+        try {
             DefaultAttribute<?> curr = head;
             for (;;) {
                 final DefaultAttribute<?> next = curr.next;
@@ -245,6 +253,8 @@ public <T> boolean remove(AttributeKey<T> key) {
                 }
                 curr = next;
             }
+        } finally {
+            head.unlock();
         }
     }
 
@@ -368,7 +378,7 @@ private enum SetAttrMode {
     }
 
     @VisibleForTesting
-    static final class DefaultAttribute<T> implements Entry<AttributeKey<T>, T> {
+    static final class DefaultAttribute<T> extends ReentrantShortLock implements Entry<AttributeKey<T>, T> {
 
         @Nullable
         private final AttributeKey<T> 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
@@ -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<Class<?>, Object> singletons = new HashMap<>();
+
+    private final Lock lock = new ReentrantShortLock();
     private boolean isShutdown;
 
     DefaultDependencyInjector(Iterable<Object> singletons) {
@@ -41,30 +46,40 @@ final class DefaultDependencyInjector implements DependencyInjector {
     }
 
     @Override
-    public synchronized <T> T getInstance(Class<T> 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> T getInstance(Class<T> 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
@@ -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<RequestLogFuture> pendingFutures = new ArrayList<>(4);
+
+    private final Lock lock = new ReentrantShortLock();
     @Nullable
     private UnmodifiableFuture<RequestLog> partiallyCompletedFuture;
     @Nullable
@@ -342,9 +349,12 @@ private <T extends RequestOnlyLog> CompletableFuture<T> 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<RequestLogFuture> pendingFutures,
+                                                             int flags) {
         if (pendingFutures.isEmpty()) {
             return null;
         }
 
-        final int flags = this.flags;
         final int maxNumListeners = pendingFutures.size();
         final Iterator<RequestLogFuture> 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
@@ -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