Merge pull request DataDog#3848 from DataDog/jpbempel/fix-race-remote-config

jpbempel · web-flow · commit 4568db60683f · 2022-09-19T13:17:35.000+02:00
Fix race condition between RC and debugger
diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java
@@ -378,12 +378,10 @@ public void execute() {
       installDatadogTracer(scoClass, sco);
       maybeStartAppSec(scoClass, sco);
       maybeStartIast(scoClass, sco);
+      // start debugger before remote config to subscribe to it before starting to poll
+      maybeStartDebugger(instrumentation, scoClass, sco);
       maybeStartRemoteConfig(scoClass, sco);
 
-      if (debuggerEnabled) {
-        startDebuggerAgent(instrumentation, scoClass, sco);
-      }
-
       if (telemetryEnabled) {
         startTelemetry(instrumentation, scoClass, sco);
       }
@@ -765,8 +763,19 @@ private static void shutdownProfilingAgent(final boolean sync) {
     }
   }
 
+  private static void maybeStartDebugger(Instrumentation inst, Class<?> scoClass, Object sco) {
+    if (!debuggerEnabled) {
+      return;
+    }
+    if (!remoteConfigEnabled) {
+      log.warn("Cannot enable Dynamic Instrumentation because Remote Configuration is not enabled");
+      return;
+    }
+    startDebuggerAgent(inst, scoClass, sco);
+  }
+
   private static synchronized void startDebuggerAgent(
-      final Instrumentation inst, Class<?> scoClass, Object sco) {
+      Instrumentation inst, Class<?> scoClass, Object sco) {
     final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader();
     try {
       Thread.currentThread().setContextClassLoader(AGENT_CLASSLOADER);
diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java
@@ -87,7 +87,6 @@ public static synchronized void run(
     configurationPoller = (ConfigurationPoller) sco.configurationPoller(config);
     if (configurationPoller != null) {
       subscribeConfigurationPoller(configurationUpdater);
-      configurationPoller.start();
 
       try {
         /*
@@ -99,6 +98,8 @@ public static synchronized void run(
       } catch (final IllegalStateException ex) {
         // The JVM is already shutting down.
       }
+    } else {
+      log.debug("No configuration poller available from SharedCommunicationObjects");
     }
   }
 
diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerAgentTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/DebuggerAgentTest.java
@@ -13,6 +13,7 @@
 import com.datadog.debugger.util.RemoteConfigHelper;
 import datadog.common.container.ContainerInfo;
 import datadog.communication.ddagent.SharedCommunicationObjects;
+import datadog.remoteconfig.ConfigurationPoller;
 import datadog.trace.api.Config;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -119,7 +120,11 @@ public void runEnabledWithDatadogAgent() throws InterruptedException, IOExceptio
     } catch (IOException e) {
       e.printStackTrace();
     }
-    DebuggerAgent.run(inst, new SharedCommunicationObjects());
+    SharedCommunicationObjects sharedCommunicationObjects = new SharedCommunicationObjects();
+    DebuggerAgent.run(inst, sharedCommunicationObjects);
+    ConfigurationPoller configurationPoller =
+        (ConfigurationPoller) sharedCommunicationObjects.configurationPoller(Config.get());
+    configurationPoller.start();
     RecordedRequest request = datadogAgentServer.takeRequest(5, TimeUnit.SECONDS);
     assertNotNull(request);
     assertEquals("/info", request.getPath());
diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java
@@ -116,7 +116,6 @@ public void init() {
   public void maybeInitPoller() {
     if (!hasUserConfig && this.configurationPoller != null) {
       subscribeConfigurationPoller();
-      configurationPoller.start();
     }
   }
 
diff --git a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/DebuggerIntegrationTest.java b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/DebuggerIntegrationTest.java
@@ -111,7 +111,8 @@ void testDestroy() throws Exception {
         new MockResponse()
             .setHeadersDelay(REQUEST_WAIT_TIMEOUT * 2, TimeUnit.SECONDS)
             .setResponseCode(200));
-    targetProcess = createProcessBuilder(logFilePath, METHOD_NAME, "2").start();
+    // wait for 3 snapshots (2 status + 1 snapshot)
+    targetProcess = createProcessBuilder(logFilePath, METHOD_NAME, "3").start();
 
     RecordedRequest request = retrieveSnapshotRequest();
     assertNotNull(request);

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,6 @@ public static synchronized void run(`
`87`	`87`	`configurationPoller = (ConfigurationPoller) sco.configurationPoller(config);`
`88`	`88`	`if (configurationPoller != null) {`
`89`	`89`	`subscribeConfigurationPoller(configurationUpdater);`
`90`		`- configurationPoller.start();`
`91`	`90`
`92`	`91`	`try {`
`93`	`92`	`/*`
`@@ -99,6 +98,8 @@ public static synchronized void run(`
`99`	`98`	`} catch (final IllegalStateException ex) {`
`100`	`99`	`// The JVM is already shutting down.`
`101`	`100`	`}`
	`101`	`+ } else {`
	`102`	`+ log.debug("No configuration poller available from SharedCommunicationObjects");`
`102`	`103`	`}`
`103`	`104`	`}`
`104`	`105`
Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,6 @@ public void init() {`
`116`	`116`	`public void maybeInitPoller() {`
`117`	`117`	`if (!hasUserConfig && this.configurationPoller != null) {`
`118`	`118`	`subscribeConfigurationPoller();`
`119`		`- configurationPoller.start();`
`120`	`119`	`}`
`121`	`120`	`}`
`122`	`121`