[DI] Fix race condition when adding probes at same location (#5619)

If two probes were added at the same location in the code in quick
succession, a race condition might occur in which the code thought there
were no existing breakpoint at that location when the 2nd probe was
about to be added. At that point it would try to add a new breakpoint
which would fail since a breakpoint had already been added there by the
first probe.

Author: watson

LICENSE-3rdparty.csv

@@ -20,6 +20,7 @@ require,limiter,MIT,Copyright 2011 John Hurliman
 require,lodash.sortby,MIT,Copyright JS Foundation and other contributors
 require,lru-cache,ISC,Copyright (c) 2010-2022 Isaac Z. Schlueter and Contributors
 require,module-details-from-path,MIT,Copyright 2016 Thomas Watson Steen
+require,mutexify,MIT,Copyright (c) 2014 Mathias Buus
 require,opentracing,MIT,Copyright 2016 Resonance Labs Inc
 require,path-to-regexp,MIT,Copyright 2014 Blake Embrey
 require,pprof-format,MIT,Copyright 2022 Stephen Belanger

package.json

@@ -106,6 +106,7 @@
     "lodash.sortby": "^4.7.0",
     "lru-cache": "^7.14.0",
     "module-details-from-path": "^1.0.3",
+    "mutexify": "^1.4.0",
     "opentracing": ">=0.12.1",
     "path-to-regexp": "^0.1.12",
     "pprof-format": "^2.1.0",

packages/dd-trace/src/debugger/devtools_client/breakpoints.js

@@ -1,7 +1,7 @@
 'use strict'
 
+const lock = require('mutexify/promise')()
 const { getGeneratedPosition } = require('./source-maps')
-const lock = require('./lock')()
 const session = require('./session')
 const { compile: compileCondition, compileSegments, templateRequiresEvaluation } = require('./condition')
 const { MAX_SNAPSHOTS_PER_SECOND_PER_PROBE, MAX_NON_SNAPSHOTS_PER_SECOND_PER_PROBE } = require('./defaults')
@@ -64,14 +64,14 @@ async function addBreakpoint (probe) {
   const release = await lock()
 
   try {
-    log.debug(
-      '[debugger:devtools_client] Adding breakpoint at %s:%d:%d (probe: %s, version: %d)',
-      url, lineNumber, columnNumber, probe.id, probe.version
-    )
-
     const locationKey = generateLocationKey(scriptId, lineNumber, columnNumber)
     const breakpoint = locationToBreakpoint.get(locationKey)
 
+    log.debug(
+      '[debugger:devtools_client] %s breakpoint at %s:%d:%d (probe: %s, version: %d)',
+      breakpoint ? 'Updating' : 'Adding', url, lineNumber, columnNumber, probe.id, probe.version
+    )
+
     if (breakpoint) {
       // A breakpoint already exists at this location, so we need to add the probe to the existing breakpoint
       await updateBreakpoint(breakpoint, probe)
@@ -82,10 +82,15 @@ async function addBreakpoint (probe) {
         lineNumber: lineNumber - 1, // Beware! lineNumber is zero-indexed
         columnNumber
       }
-      const result = await session.post('Debugger.setBreakpoint', {
-        location,
-        condition: probe.condition
-      })
+      let result
+      try {
+        result = await session.post('Debugger.setBreakpoint', {
+          location,
+          condition: probe.condition
+        })
+      } catch (err) {
+        throw new Error(`Error setting breakpoint for probe ${probe.id}`, { cause: err })
+      }
       probeToLocation.set(probe.id, locationKey)
       locationToBreakpoint.set(locationKey, { id: result.breakpointId, location, locationKey })
       breakpointToProbes.set(result.breakpointId, new Map([[probe.id, probe]]))
@@ -120,7 +125,11 @@ async function removeBreakpoint ({ id }) {
       if (breakpointToProbes.size === 0) {
         await stop() // TODO: Will this actually delete the breakpoint?
       } else {
-        await session.post('Debugger.removeBreakpoint', { breakpointId: breakpoint.id })
+        try {
+          await session.post('Debugger.removeBreakpoint', { breakpointId: breakpoint.id })
+        } catch (err) {
+          throw new Error(`Error removing breakpoint for probe ${id}`, { cause: err })
+        }
       }
     } else {
       await updateBreakpoint(breakpoint)
@@ -144,12 +153,21 @@ async function updateBreakpoint (breakpoint, probe) {
   const condition = compileCompoundCondition(Array.from(probesAtLocation.values()))
 
   if (condition || conditionBeforeNewProbe !== condition) {
-    await session.post('Debugger.removeBreakpoint', { breakpointId: breakpoint.id })
+    try {
+      await session.post('Debugger.removeBreakpoint', { breakpointId: breakpoint.id })
+    } catch (err) {
+      throw new Error(`Error removing breakpoint for probe ${probe.id}`, { cause: err })
+    }
     breakpointToProbes.delete(breakpoint.id)
-    const result = await session.post('Debugger.setBreakpoint', {
-      location: breakpoint.location,
-      condition
-    })
+    let result
+    try {
+      result = await session.post('Debugger.setBreakpoint', {
+        location: breakpoint.location,
+        condition
+      })
+    } catch (err) {
+      throw new Error(`Error setting breakpoint for probe ${probe.id}`, { cause: err })
+    }
     breakpoint.id = result.breakpointId
     breakpointToProbes.set(result.breakpointId, probesAtLocation)
   }

packages/dd-trace/src/debugger/devtools_client/lock.js

@@ -1,7 +1,7 @@
 'use strict'
 
 const { workerData: { rcPort } } = require('node:worker_threads')
-const lock = require('./lock')()
+const lock = require('mutexify/promise')()
 const { addBreakpoint, removeBreakpoint } = require('./breakpoints')
 const { ackReceived, ackInstalled, ackError } = require('./status')
 const log = require('../../log')

packages/dd-trace/src/debugger/devtools_client/remote_config.js

@@ -3212,6 +3212,13 @@ multer@^1.4.5-lts.1:
     type-is "^1.6.4"
     xtend "^4.0.0"
 
+mutexify@^1.4.0:
+  version "1.4.0"
+  resolved "https://registry.yarnpkg.com/mutexify/-/mutexify-1.4.0.tgz#b7f4ac0273c81824b840887c6a6e0bfab14bbe94"
+  integrity sha512-pbYSsOrSB/AKN5h/WzzLRMFgZhClWccf2XIB4RSMC8JbquiB0e0/SH5AIfdQMdyHmYtv4seU7yV/TvAwPLJ1Yg==
+  dependencies:
+    queue-tick "^1.0.0"
+
 natural-compare@^1.4.0:
   version "1.4.0"
   resolved "https://registry.yarnpkg.com/natural-compare/-/natural-compare-1.4.0.tgz#4abebfeed7541f2c27acfb29bdbbd15c8d5ba4f7"
@@ -3687,6 +3694,11 @@ qs@6.13.0:
   dependencies:
     side-channel "^1.0.6"
 
+queue-tick@^1.0.0:
+  version "1.0.1"
+  resolved "https://registry.yarnpkg.com/queue-tick/-/queue-tick-1.0.1.tgz#f6f07ac82c1fd60f82e098b417a80e52f1f4c142"
+  integrity sha512-kJt5qhMxoszgU/62PLP1CJytzd2NKetjSRnyuj31fDd3Rlcz3fzlFdFLD1SItunPwyqEOkca6GbV612BWfaBag==
+
 rambda@^7.4.0:
   version "7.5.0"
   resolved "https://registry.yarnpkg.com/rambda/-/rambda-7.5.0.tgz#1865044c59bc0b16f63026c6e5a97e4b1bbe98fe"