Convert synchronized to use semaphores (#4284)

* Convert to use locks instead of `synchonized` Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
helidon-io · Jun 6, 2022 · 09d1207 · 09d1207
1 parent 156b28b
commit 09d1207
Show file tree

Hide file tree

Showing 10 changed files with 262 additions and 121 deletions.
diff --git a/grpc/server/src/main/java/io/helidon/grpc/server/ProtoReflectionService.java b/grpc/server/src/main/java/io/helidon/grpc/server/ProtoReflectionService.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2021, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -47,6 +47,9 @@
 import java.util.Queue;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
 
 import com.google.protobuf.Any;
 import com.google.protobuf.DescriptorProtos;
@@ -88,7 +91,7 @@
  */
 public final class ProtoReflectionService extends ServerReflectionGrpc.ServerReflectionImplBase {
 
-    private final Object lock = new Object();
+    private final Lock indexAccess = new ReentrantLock(true);
 
     private final Map<Server, ServerReflectionIndex> serverReflectionIndexes = new WeakHashMap<>();
 
@@ -110,7 +113,7 @@ public static BindableService newInstance() {
      * mutable services or a change in the service names.
      */
     private ServerReflectionIndex getRefreshedIndex() {
-        synchronized (lock) {
+        return accessIndex(() -> {
             Server server = InternalServer.SERVER_CONTEXT_KEY.get();
             ServerReflectionIndex index = serverReflectionIndexes.get(server);
             if (index == null) {
@@ -147,7 +150,7 @@ private ServerReflectionIndex getRefreshedIndex() {
             }
 
             return index;
-        }
+        });
     }
 
     @Override
@@ -594,4 +597,13 @@ private void processExtension(FieldDescriptor extension, FileDescriptor fd) {
             fileDescriptorsByExtensionAndNumber.get(extensionName).put(extensionNumber, fd);
         }
     }
+
+    private <T> T accessIndex(Supplier<T> operation) {
+        indexAccess.lock();
+        try {
+            return operation.get();
+        } finally {
+            indexAccess.unlock();
+        }
+    }
 }
diff --git a/grpc/server/src/test/java/io/helidon/grpc/server/GrpcServiceTest.java b/grpc/server/src/test/java/io/helidon/grpc/server/GrpcServiceTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,6 +24,8 @@
 import java.util.concurrent.CompletionStage;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.stream.Stream;
@@ -511,9 +513,21 @@ public void update(ServiceDescriptor.Rules rules) {
      */
     private static class SynchronizedObserver<T>
             extends TestStreamObserver<T> {
+
+        private final Lock nextAccess = new ReentrantLock(true);
+
         @Override
-        public synchronized void onNext(T t) {
-            super.onNext(t);
+        public void onNext(T t) {
+            accessNext(() -> super.onNext(t));
+        }
+
+        private void accessNext(Runnable operation) {
+            nextAccess.lock();
+            try {
+                operation.run();
+            } finally {
+                nextAccess.unlock();
+            }
         }
     }
 }
diff --git a/metrics/api/src/main/java/io/helidon/metrics/api/RegistryFactoryManager.java b/metrics/api/src/main/java/io/helidon/metrics/api/RegistryFactoryManager.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2021, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,9 @@
 package io.helidon.metrics.api;
 
 import java.util.ServiceLoader;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -83,6 +86,8 @@ private static RegistryFactoryProvider loadRegistryFactoryProvider() {
         return provider;
     }
 
+    private static final Lock SETTINGS_ACCESS = new ReentrantLock(true);
+
     private RegistryFactoryManager() {
     }
 
@@ -105,21 +110,34 @@ static RegistryFactory getInstance() {
         return INSTANCE.get();
     }
 
-    static synchronized RegistryFactory getInstance(MetricsSettings metricsSettings) {
-        RegistryFactoryManager.metricsSettings = metricsSettings;
-        RegistryFactory result = INSTANCE.get();
-        result.update(metricsSettings);
-        return result;
+    static RegistryFactory getInstance(MetricsSettings metricsSettings) {
+
+        return accessMetricsSettings(() -> {
+            RegistryFactoryManager.metricsSettings = metricsSettings;
+            RegistryFactory result = INSTANCE.get();
+            result.update(metricsSettings);
+            return result;
+        });
     }
 
-    static synchronized RegistryFactory getInstance(ComponentMetricsSettings componentMetricsSettings) {
-        return componentMetricsSettings.isEnabled()
+    static RegistryFactory getInstance(ComponentMetricsSettings componentMetricsSettings) {
+
+        return accessMetricsSettings(() -> componentMetricsSettings.isEnabled()
                 ? INSTANCE.get()
-                : NO_OP_INSTANCE;
+                : NO_OP_INSTANCE);
     }
 
     @Deprecated
     static RegistryFactory getInstance(Config config) {
         return getInstance(MetricsSettings.create(config));
     }
+
+    private static <T> T accessMetricsSettings(Supplier<T> operation) {
+        SETTINGS_ACCESS.lock();
+        try {
+            return operation.get();
+        } finally {
+            SETTINGS_ACCESS.unlock();
+        }
+    }
 }
diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/PeriodicExecutor.java b/metrics/metrics/src/main/java/io/helidon/metrics/PeriodicExecutor.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2021, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,8 +21,9 @@
 import java.util.Set;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -32,10 +33,6 @@
  * Some enrollments might arrive before the manager is started. We save those and act on them once the
  * manager starts. This makes sure the executor's thread starts only at native image runtime.
  * </p>
- * <p>
- * In production use, starting and stopping the executor and even enrolling callbacks are not performance-critical operations,
- * so simple synchronization on methods which access shared data is clear and sufficient.
- * </p>
  */
 class PeriodicExecutor {
 
@@ -81,7 +78,7 @@ private static class Enrollment {
 
     private final Collection<Enrollment> deferredEnrollments = new ArrayList<>();
 
-    private final Semaphore access = new Semaphore(1, true);
+    private final Lock access = new ReentrantLock(true);
 
     private PeriodicExecutor() {
     }
@@ -173,12 +170,11 @@ State executorState() {
     }
 
     private void sync(String taskDescription, Runnable task) {
+        access.lock();
         try {
-            access.acquire();
             task.run();
-            access.release();
-        } catch (InterruptedException ex) {
-            LOGGER.log(Level.WARNING, "Attempt to " + taskDescription + " failed", ex);
+        } finally {
+            access.unlock();
         }
     }
 }
diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/RegistryFactory.java b/metrics/metrics/src/main/java/io/helidon/metrics/RegistryFactory.java
@@ -17,7 +17,8 @@
 package io.helidon.metrics;
 
 import java.util.EnumMap;
-import java.util.concurrent.Semaphore;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import io.helidon.config.Config;
 import io.helidon.metrics.api.MetricsSettings;
@@ -45,7 +46,7 @@
 public class RegistryFactory implements io.helidon.metrics.api.RegistryFactory {
 
     private final EnumMap<Type, Registry> registries = new EnumMap<>(Type.class);
-    private final Semaphore metricsSettingsAccess = new Semaphore(1);
+    private final Lock metricsSettingsAccess = new ReentrantLock(true);
     private MetricsSettings metricsSettings;
 
     /**
@@ -68,12 +69,11 @@ private RegistryFactory(MetricsSettings metricsSettings) {
     }
 
     private void accessMetricsSettings(Runnable operation) {
+        metricsSettingsAccess.lock();
         try {
-            metricsSettingsAccess.acquire();
             operation.run();
-            metricsSettingsAccess.release();
-        } catch (InterruptedException e) {
-            throw new RuntimeException(e);
+        } finally {
+            metricsSettingsAccess.unlock();
         }
     }
 
@@ -164,7 +164,7 @@ public void update(MetricsSettings metricsSettings) {
         });
     }
 
-    private synchronized void ensureBase() {
+    private void ensureBase() {
         if (null == registries.get(Type.BASE)) {
             accessMetricsSettings(() -> {
                 Registry registry = BaseRegistry.create(metricsSettings);

diff --git a/microprofile/cdi/src/main/java/io/helidon/microprofile/cdi/BuildTimeInitializer.java b/microprofile/cdi/src/main/java/io/helidon/microprofile/cdi/BuildTimeInitializer.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2020 Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,6 +15,10 @@
  */
 package io.helidon.microprofile.cdi;
 
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
+
 import io.helidon.common.LogConfig;
 
 /**
@@ -25,6 +29,8 @@
 final class BuildTimeInitializer {
     private static volatile HelidonContainerImpl container;
 
+    private static final Lock CONTAINER_ACCESS = new ReentrantLock(true);
+
     static {
         // need to initialize logging as soon as possible
         LogConfig.initClass();
@@ -34,21 +40,38 @@ final class BuildTimeInitializer {
     private BuildTimeInitializer() {
     }
 
-    static synchronized HelidonContainerImpl get() {
-        if (null == container) {
-            createContainer();
-        }
+    static HelidonContainerImpl get() {
+        return accessContainer(() -> {
+            if (null == container) {
+                createContainer();
+            }
 
-        return container;
+            return container;
+        });
     }
 
-    static synchronized void reset() {
-        container = null;
+    static void reset() {
+        accessContainer(() -> {
+            container = null;
+            return null;
+        });
     }
 
     private static void createContainer() {
         // static initialization to support GraalVM native image
-        container = HelidonContainerImpl.create();
-        ContainerInstanceHolder.addListener(BuildTimeInitializer::reset);
+        accessContainer(() -> {
+            container = HelidonContainerImpl.create();
+            ContainerInstanceHolder.addListener(BuildTimeInitializer::reset);
+            return null;
+        });
+    }
+
+    private static <T> T accessContainer(Supplier<T> operation) {
+        CONTAINER_ACCESS.lock();
+        try {
+            return operation.get();
+        } finally {
+            CONTAINER_ACCESS.unlock();
+        }
     }
 }