From e117bd3cb7c14982ad016ec326a388cef53f1ed8 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 09:54:33 -0400 Subject: [PATCH 1/3] fix: memoize channel(name) to make Channel identity stable per name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The phony AVOID_GARBAGE_COLLECTION subscriber installed by this patch keeps a created Channel alive, but Node's underlying channel(name) can still return a brand-new Channel object for the same name on a subsequent call (observed on Node 18 under certain workloads — e.g. heavy proxyquire reload + plugin teardown/recreate cycles). Because the existing anti-GC tracking is a WeakSet keyed by Channel-object identity, each new object is treated as new and gets its own phony subscriber. The two objects are never unified. The visible failure mode for callers: code that captures a Channel in a module-level closure (a common pattern for instrumentation publishers) ends up publishing to a different Channel than later subscribers attach to. Publishes go nowhere; subscribers see nothing. hasSubscribers reports false (only the phony is on the publish-side Channel) and traceSync/traceCallback fast-path-bypass the publish. This fix memoizes dc.channel(name) by name and short-circuits before delegating to channel() on subsequent calls. Channel identity is now stable per name regardless of what the underlying channel() returns — the contract callers actually depend on. Empirically verified end-to-end against dd-trace-js's ws plugin stress workflow (10 runners × 25 iterations of plugin tests on Node 18.20.8, 250 runs total): 0/10 runners pass without this patch, 10/10 pass with it. dd-trace-js PR for context: DataDog/dd-trace-js#8297. All existing dc-polyfill tests pass (26/26 suites). --- patch-garbage-collection-bug.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/patch-garbage-collection-bug.js b/patch-garbage-collection-bug.js index 0615ebf..0ad888a 100644 --- a/patch-garbage-collection-bug.js +++ b/patch-garbage-collection-bug.js @@ -1,5 +1,16 @@ // There's a bug where a newly created channel is immediately garbage collected // @see https://github.com/nodejs/node/pull/47520 +// +// The phony subscriber below keeps a created channel alive, but Node's channel(name) +// can still return a brand-new Channel object for the same name on a subsequent call +// (observed on Node 18 under certain workloads). A WeakSet keyed by Channel-object +// identity can't merge those — each new object is treated as new and gets its own +// phony subscriber. Callers that captured the earlier Channel in a closure end up +// publishing to a different object than later subscribe()-ers attach to. +// +// To make channel identity stable per name (the contract callers expect), memoize +// dc.channel(name) by name and short-circuit before delegating to the underlying +// channel() on subsequent calls. const PHONY_SUBSCRIBE = function AVOID_GARBAGE_COLLECTION() {}; const { @@ -10,11 +21,15 @@ const { module.exports = function(unpatched) { const dc_channel = unpatched.channel; const channels = new WeakSet(); + const byName = new Map(); const dc = { ...unpatched }; dc.channel = function() { + const name = arguments[0]; + if (byName.has(name)) return byName.get(name); const ch = dc_channel.apply(this, arguments); + byName.set(name, ch); if (channels.has(ch)) return ch; From fd0572f3bd21c835f851fc0ddd6394e2fafbdffd Mon Sep 17 00:00:00 2001 From: bm1549 Date: Fri, 8 May 2026 10:02:53 -0400 Subject: [PATCH 2/3] test: regression test for memoized channel(name) identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a unit test that injects a mock unpatched module whose channel(name) returns a different Channel object on every call, simulating the Node 18 bug. Without the memoization in patch-garbage-collection-bug.js, this test fails 6/8 assertions (channel identity diverges, subscribers attach to a different Channel than later publishers, the underlying channel() is re-invoked on every memoizable lookup). With the patch, all 8 pass. This pins the contract the patch is meant to enforce — dc.channel(name) returns a stable Channel object per name regardless of what the underlying registry hands back — without requiring a real Node 18 bug to be triggered to exercise the patch. --- test/garbage-collection-patch.spec.js | 83 +++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 test/garbage-collection-patch.spec.js diff --git a/test/garbage-collection-patch.spec.js b/test/garbage-collection-patch.spec.js new file mode 100644 index 0000000..c027eeb --- /dev/null +++ b/test/garbage-collection-patch.spec.js @@ -0,0 +1,83 @@ +const test = require('tape'); +const patch = require('../patch-garbage-collection-bug.js'); + +// Simulate the Node 18 bug: the underlying channel(name) returns a +// brand-new Channel object on every call for the same name, even when +// the previous one is still held alive by JS code. The patch's job is +// to make dc.channel(name) return a stable Channel identity per name +// regardless of what the underlying registry returns. +function mockUnpatched() { + const calls = { count: 0 }; + function channel(name) { + calls.count++; + return { + _subscribers: [], + _stores: new Map(), + _name: name, + _instanceId: calls.count, + subscribe(fn) { this._subscribers.push(fn); }, + unsubscribe(fn) { + const i = this._subscribers.indexOf(fn); + if (i >= 0) this._subscribers.splice(i, 1); + return i >= 0; + }, + publish(data) { + for (const sub of this._subscribers) sub(data); + } + }; + } + return { channel, calls }; +} + +test('garbage-collection patch: dc.channel(name) returns stable identity across calls', t => { + const { channel, calls } = mockUnpatched(); + const dc = patch({ channel }); + + const a = dc.channel('foo'); + const b = dc.channel('foo'); + const c = dc.channel('foo'); + + t.strictEqual(a, b, 'second call to dc.channel(name) returns same Channel object'); + t.strictEqual(b, c, 'third call returns same Channel object'); + t.ok(calls.count >= 1, 'underlying channel() was called at least once for first lookup'); + + const callsAfterMemoization = calls.count; + dc.channel('foo'); + dc.channel('foo'); + t.equal(calls.count, callsAfterMemoization, + 'memoized lookups do not re-invoke the underlying channel()'); + + t.end(); +}); + +test('garbage-collection patch: distinct names get distinct Channel objects', t => { + const { channel } = mockUnpatched(); + const dc = patch({ channel }); + + const foo = dc.channel('foo'); + const bar = dc.channel('bar'); + + t.notStrictEqual(foo, bar, 'different names return different Channel objects'); + t.strictEqual(dc.channel('foo'), foo, 'foo memoization holds'); + t.strictEqual(dc.channel('bar'), bar, 'bar memoization holds'); + t.end(); +}); + +test('garbage-collection patch: subscribers attach to the memoized Channel and receive publishes', t => { + // This is the end-to-end shape the bug produces: the publisher captures + // a Channel from one call, the subscriber attaches via a later call. + // Without memoization (when the underlying channel() misbehaves) the + // two see different Channel objects and the subscriber never fires. + const { channel } = mockUnpatched(); + const dc = patch({ channel }); + + const publisher = dc.channel('observable'); + + let received = null; + dc.channel('observable').subscribe((data) => { received = data; }); + + publisher.publish('hello'); + + t.equal(received, 'hello', 'subscriber attached to a later dc.channel(name) lookup receives publishes from the earlier-captured Channel'); + t.end(); +}); From deac8942cc8889cba1f93869f66df71ec8c4c42f Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Fri, 8 May 2026 10:12:55 -0400 Subject: [PATCH 3/3] Apply suggestion from @bm1549 --- patch-garbage-collection-bug.js | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/patch-garbage-collection-bug.js b/patch-garbage-collection-bug.js index 0ad888a..e7bdf07 100644 --- a/patch-garbage-collection-bug.js +++ b/patch-garbage-collection-bug.js @@ -1,16 +1,5 @@ // There's a bug where a newly created channel is immediately garbage collected // @see https://github.com/nodejs/node/pull/47520 -// -// The phony subscriber below keeps a created channel alive, but Node's channel(name) -// can still return a brand-new Channel object for the same name on a subsequent call -// (observed on Node 18 under certain workloads). A WeakSet keyed by Channel-object -// identity can't merge those — each new object is treated as new and gets its own -// phony subscriber. Callers that captured the earlier Channel in a closure end up -// publishing to a different object than later subscribe()-ers attach to. -// -// To make channel identity stable per name (the contract callers expect), memoize -// dc.channel(name) by name and short-circuit before delegating to the underlying -// channel() on subsequent calls. const PHONY_SUBSCRIBE = function AVOID_GARBAGE_COLLECTION() {}; const {