diagnostics_channel: fix diagnostics channel memory leak

[theanarkh]

fix diagnostics channel memory leak.
cc @Qard

const { subscribe, unsubscribe } = require('diagnostics_channel');

function noop() {}

console.log(process.memoryUsage().heapUsed);

for (let i = 0; i < 1000000; i++) {
    subscribe(String(i), noop);
    unsubscribe(String(i), noop);
}

gc();

console.log(process.memoryUsage().heapUsed);

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Qard]

LGTM. Not really how users are intended to use diagnostics_channel, but how users are meant to use things and how they actually use them doesn't always match, so seems like a reasonable fix to me. 😅

[anonrig]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48203/

[RaisinTen]

Could you add a test for this? Maybe by comparing the values of process.memoryUsage().heapUsed before and after the loop and gc call?

[theanarkh]

Could you add a test for this? Maybe by comparing the values of process.memoryUsage().heapUsed before and after the loop and gc call?

I think it is difficult to do that, do you have any idea? Thanks!

[RaisinTen]

I think it is difficult to do that, do you have any idea? Thanks!

I meant using the code you shared in the PR description with a slight modification - asserting that the value of process.memoryUsage().heapUsed after the loop+gc is less than or equal to the value before. Like this:

// Flags: --expose-gc
'use strict';

// This test ensures that diagnostic channel references aren't leaked.

require('../common');
const { ok } = require('assert');

const { subscribe, unsubscribe } = require('diagnostics_channel');

function noop() {}

const heapUsedBefore = process.memoryUsage().heapUsed;

for (let i = 0; i < 1000000; i++) {
    subscribe(String(i), noop);
    unsubscribe(String(i), noop);
}

gc();

const heapUsedAfter = process.memoryUsage().heapUsed;

ok(heapUsedBefore >= heapUsedAfter);

Is it difficult because it's too flaky?

[theanarkh]

Thanks for the suggestion. When i add the test(i = 1000000), i find the output is as follows.

before gc 5350312
after  gc 15322320

But when i change the code channels[name] = null; to delete channels[name];, the output is as follows.

before gc 5353152
after  gc 4639440

Then i take the snapshot before and after the loop. I find It takes up ten more megabytes of memory in channels[name] = null;(But when i is 100, the output of two cases is the same) 😄.

[RaisinTen]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48229/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48231/

[nodejs-github-bot]

Landed in d579bc1