diagnostics_channel: fix diagnostics channel memory leak

[theanarkh]

fix diagnostics channel memory leak.
cc @Qard

const{ subscribe, unsubscribe }=require('diagnostics_channel');functionnoop(){}console.log(process.memoryUsage().heapUsed);for(leti=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');functionnoop(){}constheapUsedBefore=process.memoryUsage().heapUsed;for(leti=0;i<1000000;i++){subscribe(String(i),noop);unsubscribe(String(i),noop);}gc();constheapUsedAfter=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