Remove Guice + use Singleton to fix initilization potential deadlock (j…

…enkinsci#682)

src/main/java/org/jenkinsci/plugins/prometheus/rest/PrometheusAction.java

@@ -1,13 +1,13 @@
 package org.jenkinsci.plugins.prometheus.rest;
 
-import com.google.inject.Inject;
 import hudson.Extension;
 import hudson.model.UnprotectedRootAction;
 import hudson.util.HttpResponses;
 import io.prometheus.client.exporter.common.TextFormat;
 import jenkins.metrics.api.Metrics;
 import jenkins.model.Jenkins;
 import org.jenkinsci.plugins.prometheus.config.PrometheusConfiguration;
+import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics;
 import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.StaplerRequest;
@@ -16,12 +16,7 @@
 @Extension
 public class PrometheusAction implements UnprotectedRootAction {
 
-    private PrometheusMetrics prometheusMetrics;
-
-    @Inject
-    public void setPrometheusMetrics(PrometheusMetrics prometheusMetrics) {
-        this.prometheusMetrics = prometheusMetrics;
-    }
+    private final PrometheusMetrics prometheusMetrics = DefaultPrometheusMetrics.get();
 
     @Override
     public String getIconFileName() {

src/main/java/org/jenkinsci/plugins/prometheus/service/DefaultPrometheusMetrics.java

@@ -1,15 +1,24 @@
 package org.jenkinsci.plugins.prometheus.service;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.ExtensionList;
+import hudson.init.InitMilestone;
+import hudson.init.Initializer;
 import io.prometheus.client.Collector;
 import io.prometheus.client.CollectorRegistry;
 import io.prometheus.client.dropwizard.DropwizardExports;
 import io.prometheus.client.exporter.common.TextFormat;
 import io.prometheus.client.hotspot.DefaultExports;
 import jenkins.metrics.api.Metrics;
-import org.jenkinsci.plugins.prometheus.*;
+import org.jenkinsci.plugins.prometheus.CodeCoverageCollector;
+import org.jenkinsci.plugins.prometheus.DiskUsageCollector;
+import org.jenkinsci.plugins.prometheus.ExecutorCollector;
+import org.jenkinsci.plugins.prometheus.JenkinsStatusCollector;
+import org.jenkinsci.plugins.prometheus.JobCollector;
 import org.jenkinsci.plugins.prometheus.config.disabledmetrics.FilteredMetricEnumeration;
 import org.jenkinsci.plugins.prometheus.util.JenkinsNodeBuildsSampleBuilder;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -21,27 +30,51 @@ public class DefaultPrometheusMetrics implements PrometheusMetrics {
 
     private static final Logger logger = LoggerFactory.getLogger(DefaultPrometheusMetrics.class);
 
+    private static DefaultPrometheusMetrics INSTANCE = null;
+
     private final CollectorRegistry collectorRegistry;
     private final AtomicReference<String> cachedMetrics;
 
-    public DefaultPrometheusMetrics() {
+    private DefaultPrometheusMetrics() {
         CollectorRegistry collectorRegistry = CollectorRegistry.defaultRegistry;
-        collectorRegistry.register(new JobCollector());
-        collectorRegistry.register(new JenkinsStatusCollector());
-        collectorRegistry.register(new DropwizardExports(Metrics.metricRegistry(), new JenkinsNodeBuildsSampleBuilder()));
-        collectorRegistry.register(new DiskUsageCollector());
-        collectorRegistry.register(new ExecutorCollector());
-        collectorRegistry.register(new CodeCoverageCollector());
-
-        // other collectors from other plugins
-        ExtensionList.lookup(Collector.class).forEach(collectorRegistry::register);
-
         DefaultExports.initialize();
-
         this.collectorRegistry = collectorRegistry;
         this.cachedMetrics = new AtomicReference<>("");
     }
 
+    public static synchronized DefaultPrometheusMetrics get() {
+        if(INSTANCE == null) {
+            INSTANCE = new DefaultPrometheusMetrics();
+        }
+        return INSTANCE;
+    }
+
+    @Restricted(NoExternalUse.class)
+    private void registerCollector(@NonNull Collector collector) {
+        collectorRegistry.register(collector);
+        logger.debug(String.format("Collector %s registered", collector.getClass().getName()));
+    }
+
+    @Restricted(NoExternalUse.class)
+    @Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.JOB_LOADED)
+    public static void registerCollectors() {
+        DefaultPrometheusMetrics instance = get();
+        instance.registerCollector(new JenkinsStatusCollector());
+        instance.registerCollector(new DropwizardExports(Metrics.metricRegistry(), new JenkinsNodeBuildsSampleBuilder()));
+        instance.registerCollector(new DiskUsageCollector());
+        instance.registerCollector(new ExecutorCollector());
+    }
+
+    @Restricted(NoExternalUse.class)
+    @Initializer(after = InitMilestone.JOB_LOADED, before = InitMilestone.JOB_CONFIG_ADAPTED)
+    public static void registerJobCollectors() {
+        DefaultPrometheusMetrics instance = get();
+        instance.registerCollector(new JobCollector());
+        instance.registerCollector(new CodeCoverageCollector());
+        // other collectors from other plugins
+        ExtensionList.lookup(Collector.class).forEach(instance::registerCollector);
+    }
+
     @Override
     public String getMetrics() {
         return cachedMetrics.get();

src/main/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorker.java

@@ -1,6 +1,5 @@
 package org.jenkinsci.plugins.prometheus.service;
 
-import com.google.inject.Inject;
 import hudson.Extension;
 import hudson.model.AsyncPeriodicWork;
 import hudson.model.TaskListener;
@@ -16,17 +15,12 @@ public class PrometheusAsyncWorker extends AsyncPeriodicWork {
 
     private static final Logger logger = LoggerFactory.getLogger(PrometheusAsyncWorker.class);
 
-    private PrometheusMetrics prometheusMetrics;
+    private final PrometheusMetrics prometheusMetrics = DefaultPrometheusMetrics.get();
 
     public PrometheusAsyncWorker() {
         super("prometheus_async_worker");
     }
 
-    @Inject
-    public void setPrometheusMetrics(PrometheusMetrics prometheusMetrics) {
-        this.prometheusMetrics = prometheusMetrics;
-    }
-
     @Override
     public long getRecurrencePeriod() {
         long collectingMetricsPeriodInMillis =

src/test/java/org/jenkinsci/plugins/prometheus/rest/PrometheusActionTest.java

@@ -4,7 +4,7 @@
 import jenkins.metrics.api.Metrics;
 import jenkins.model.Jenkins;
 import org.jenkinsci.plugins.prometheus.config.PrometheusConfiguration;
-import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics;
+import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
@@ -22,8 +22,14 @@
 import java.io.PrintWriter;
 import java.io.StringWriter;
 
-import static java.net.HttpURLConnection.*;
-import static org.mockito.Mockito.*;
+import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
+import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
+import static java.net.HttpURLConnection.HTTP_OK;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
 public class PrometheusActionTest {
@@ -87,25 +93,27 @@ public void shouldThrowExceptionWhenAuthenticationEnabledAndInsufficientPermissi
     @Test
     public void shouldReturnMetrics() throws IOException, ServletException {
         // given
-        PrometheusAction action = new PrometheusAction();
-        PrometheusMetrics prometheusMetrics = mock(PrometheusMetrics.class);
+        DefaultPrometheusMetrics prometheusMetrics = mock(DefaultPrometheusMetrics.class);
         String responseBody = "testMetric";
         when(prometheusMetrics.getMetrics()).thenReturn(responseBody);
-        action.setPrometheusMetrics(prometheusMetrics);
-        StaplerRequest request = mock(StaplerRequest.class);
-        String url = "prometheus";
-        when(request.getRestOfPath()).thenReturn(url);
-
-        // when
-        HttpResponse actual = action.doDynamic(request);
-
-        // then
-        AssertStaplerResponse.from(actual)
-            .call()
-            .assertHttpStatus(HTTP_OK)
-            .assertContentType(TextFormat.CONTENT_TYPE_004)
-            .assertHttpHeader("Cache-Control", "must-revalidate,no-cache,no-store")
-            .assertBody(responseBody);
+        try (MockedStatic<DefaultPrometheusMetrics> defaultPrometheusMetricsMockedStatic = mockStatic(DefaultPrometheusMetrics.class)) {
+            defaultPrometheusMetricsMockedStatic.when(DefaultPrometheusMetrics::get).thenReturn(prometheusMetrics);
+            PrometheusAction action = new PrometheusAction();
+            StaplerRequest request = mock(StaplerRequest.class);
+            String url = "prometheus";
+            when(request.getRestOfPath()).thenReturn(url);
+
+            // when
+            HttpResponse actual = action.doDynamic(request);
+
+            // then
+            AssertStaplerResponse.from(actual)
+                .call()
+                .assertHttpStatus(HTTP_OK)
+                .assertContentType(TextFormat.CONTENT_TYPE_004)
+                .assertHttpHeader("Cache-Control", "must-revalidate,no-cache,no-store")
+                .assertBody(responseBody);
+        }
     }
 
     private static class AssertStaplerResponse {

src/test/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorkerTest.java

@@ -5,29 +5,35 @@
 import org.jvnet.hudson.test.Issue;
 import org.mockito.MockedStatic;
 
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Level;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 
 public class PrometheusAsyncWorkerTest {
 
     @Test
     public void shouldCollectMetrics() {
-        // given
-        PrometheusAsyncWorker asyncWorker = new PrometheusAsyncWorker();
-        PrometheusMetrics metrics = new TestPrometheusMetrics();
-        asyncWorker.setPrometheusMetrics(metrics);
-
-        // when
-        asyncWorker.execute(null);
-
-        // then
-        String actual = metrics.getMetrics();
-        assertEquals("1", actual);
+        try (MockedStatic<DefaultPrometheusMetrics> defaultPrometheusMetricsMockedStatic = mockStatic(DefaultPrometheusMetrics.class)) {
+            // given
+            DefaultPrometheusMetrics metrics = spy(DefaultPrometheusMetrics.class);
+            doNothing().when(metrics).collectMetrics();
+            defaultPrometheusMetricsMockedStatic.when(DefaultPrometheusMetrics::get).thenReturn(metrics);
+            PrometheusAsyncWorker asyncWorker = new PrometheusAsyncWorker();
+
+            // when
+            asyncWorker.execute(null);
+
+            // then
+            verify(metrics, times(1)).collectMetrics();
+        }
 
     }
     @Test
@@ -50,21 +56,4 @@ public void ensureLoggingLevel() {
         Level level = sut.getNormalLoggingLevel();
         assertEquals(Level.FINE, level);
     }
-
-    private static class TestPrometheusMetrics implements PrometheusMetrics {
-        private final AtomicReference<String> cachedMetrics = new AtomicReference<>("");
-        private final AtomicInteger counter = new AtomicInteger(0);
-
-        @Override
-        public String getMetrics() {
-            return cachedMetrics.get();
-        }
-
-        @Override
-        public void collectMetrics() {
-            String metrics = String.valueOf(counter.incrementAndGet());
-            cachedMetrics.set(metrics);
-        }
-
-    }
 }