From 10c9ef93bec80a34acc22d0a06d928fc5f5e3dcb Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 19 Feb 2025 11:17:32 +0100 Subject: [PATCH] [DI] Workaround bug in AsyncLocalStorage which would otherwise throw (#5290) This is a workaround for, what seems like a bug in Node.js core, that seems to trigger when, among other things, a lot of timers are being created very rapidly. This makes the call to `setTimeout` throw an error from within `AsyncLocalStorage._propagate` with the following TypeError: Cannot read properties of undefined (reading 'Symbol(kResourceStore)') --- .github/workflows/system-tests.yml | 2 +- .../debugger/devtools_client/breakpoints.js | 6 ++--- .../debugger/devtools_client/source-maps.js | 26 ++++++++++++++----- .../devtools_client/source-maps.spec.js | 4 ++- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 949d74e5b0f..fbd0152fb5a 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -31,7 +31,7 @@ jobs: uses: DataDog/system-tests/.github/workflows/compute-workflow-parameters.yml@main with: library: nodejs - scenarios_groups: essentials,appsec_rasp + scenarios_groups: essentials,appsec_rasp,debugger system-tests: runs-on: ${{ contains(fromJSON('["CROSSED_TRACING_LIBRARIES", "INTEGRATIONS"]'), matrix.scenario) && 'ubuntu-latest-16-cores' || 'ubuntu-latest' }} diff --git a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js index 229abf42df6..a13c32e2c10 100644 --- a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js +++ b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js @@ -31,9 +31,9 @@ async function addBreakpoint (probe) { probe.nsBetweenSampling = BigInt(1 / snapshotsPerSecond * 1e9) probe.lastCaptureNs = 0n - // TODO: Inbetween `await session.post('Debugger.enable')` and here, the scripts are parsed and cached. - // Maybe there's a race condition here or maybe we're guraenteed that `await session.post('Debugger.enable')` will - // not continue untill all scripts have been parsed? + // Warning: The code below relies on undocumented behavior of the inspector! + // It expects that `await session.post('Debugger.enable')` will wait for all loaded scripts to be emitted as + // `Debugger.scriptParsed` events. If this ever changes, we will have a race condition! const script = findScriptFromPartialPath(file) if (!script) throw new Error(`No loaded script found for ${file} (probe: ${probe.id}, version: ${probe.version})`) const { url, scriptId, sourceMapURL, source } = script diff --git a/packages/dd-trace/src/debugger/devtools_client/source-maps.js b/packages/dd-trace/src/debugger/devtools_client/source-maps.js index 36e12f3e5bd..203d2667019 100644 --- a/packages/dd-trace/src/debugger/devtools_client/source-maps.js +++ b/packages/dd-trace/src/debugger/devtools_client/source-maps.js @@ -7,6 +7,7 @@ const { SourceMapConsumer } = require('source-map') const cache = new Map() let cacheTimer = null +let cacheTimerLastSet = 0 const self = module.exports = { async loadSourceMap (dir, url) { @@ -33,13 +34,26 @@ const self = module.exports = { } } +// TODO: Remove if-statement around `setTimeout` below once it's safe to do so. +// +// This is a workaround for, what seems like a bug in Node.js core, that seems to trigger when, among other things, a +// lot of timers are being created very rapidly. This makes the call to `setTimeout` throw an error from within +// `AsyncLocalStorage._propagate` with the following error message: +// +// TypeError: Cannot read properties of undefined (reading 'Symbol(kResourceStore)') +// +// Source: https://github.com/nodejs/node/blob/v18.20.6/lib/async_hooks.js#L312 function cacheIt (key, value) { - clearTimeout(cacheTimer) - cacheTimer = setTimeout(function () { - // Optimize for app boot, where a lot of reads might happen - // Clear cache a few seconds after it was last used - cache.clear() - }, 10_000).unref() + const now = Date.now() + if (now > cacheTimerLastSet + 1_000) { + clearTimeout(cacheTimer) + cacheTimer = setTimeout(function () { + // Optimize for app boot, where a lot of reads might happen + // Clear cache a few seconds after it was last used + cache.clear() + }, 10_000).unref() + cacheTimerLastSet = now + } cache.set(key, value) return value } diff --git a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js index 68cbea0986c..768c482c43e 100644 --- a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js +++ b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js @@ -98,7 +98,9 @@ describe('source map utils', function () { let clock function setup () { - clock = sinon.useFakeTimers() + clock = sinon.useFakeTimers({ + toFake: ['setTimeout'] + }) readFileSync = sinon.stub().returns(rawSourceMap) readFile = sinon.stub().resolves(rawSourceMap)