memory leak when instance is created inside a function.

[overflowz]

Hi, I just noticed that when you create debug instance in a function, it is starting to leak the memory without freeing. Here's how you can reproduce it:

const debug = require('debug');

const loop = () => {
  const d = debug('namespace:that:i:want:for:this:function');
  d('hello world');
  setImmediate(loop);
};

loop();

If you run this and look at memory, it is leaking a lot without freeing them.
also does not matter if I set environment to DEBUG=* or not, it still leaks. Any thoughts?

EDIT: tested on 3.1.1 and 4.1.1 as well (had 3.1.1 version and then I upgraded to latest one to check if it was fixed).

EDIT2: using node version 10.13 and Windows 10 x64.

[overflowz]

Just found out, that you have to call destroy() on it if you want it to not leak.

I'm not sure how good this case is but, at least it should be documented like big red fancy text or something since I lost quite a lot of time to understand why my app was leaking though.

[Qix-]

This isn't a typical use-case for debug, though.

However, I agree that this shouldn't be leaking. I'll flag it as a bug.

[overflowz]

well, I found myself pretty much comfy to create instances within a functions, it helps to understand logs much better though (for me), like what function is doing what, etc. But yeah, leaks should not be happening at any cost, dunno.

[overflowz]

The leak comes from here: common.js, line 131.

Question is, does it cause too much trouble if you push there unique instances by namespace?

EDIT: I can create PR for that if you want to, though.

[Qix-]

No, that wouldn't solve it. This will get solved with #645 as the only reason why we're storing references to debug instances is to enable/disable them after they've been created.

I think there's a solution to this in the works, just not directly.

I'll leave this open for now. Good catch, though.

EDIT: Another reason why this is tricky is because javascript has no concept of "weak references". A std::weak_ptr would be what you'd use here.

[overflowz]

Yeah, I saw that and there were no other usages for it but enabling/disabling the instances.

about #645, I'm not sure, though. I'd either remove the saving part completely (which will remove enable/disable feature) or save instances uniquely by namespace (IMO it is more reasonable to have unique namespaces, since having same namespace in different places could be misleading, dunno).

EDIT: Either way, you know what is leaking atm and you know better what to do. Also you can @ me if I can help somehow as well, cheers!

[Qix-]

save instances uniquely by namespace

This could still potentially cause a memory leak if the namespace is dynamic.

At this point, the instances should be reporting their namespaces and a lookup should occur upon debugging that checks to see if it's enabled by the reporter. This is what #645 touches on, as well as #556.

[overflowz]

Oh, didn't knew about the dynamic part (not using it, hehe). So yeah, will be waiting for them.

Cheers.

[brandonros]

Just found out, that you have to call destroy() on it if you want it to not leak.

I'm not sure how good this case is but, at least it should be documented like big red fancy text or something since I lost quite a lot of time to understand why my app was leaking though.

Is there anyway we can get https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/debug/index.d.ts updated to expose the destroy method?

[Qix-]

@brandonros feel free to open a PR there.

[brandonros]

@Qix- The open source community amazes me. The time it took you to tell me to open a PR, you could have opened the PR...

[Qix-]

Hi @brandonros, I'm currently dealing with moving into my new apartment across the planet, a full time job, a new full time job on the way, and some heavy family issues.

Nobody is paying me to do any of this.

I have been primarily mobile for the last 3 weeks. I have little problem responding to issues on GH, but am relatively incapable of doing much coding.

I'm not sorry my schedule doesn't revolve around you, sir.

[ibc]

Wow! so if I create dynamic debug instances (such as in the constructor of a class) they will leak when the class instance no longer exists!

class Foo {
  constructor(id) {
    this.debug = debug(`foo:${id}`);
  }
}

const foo = new Foo(1234);

so if I create many Foo instances it will leak debug instances!

IMHO it should be clearly documented that this may happen and that a destroy() method exists to prevent that. The destroy() method is not even documented in the README. I will send a PR documenting it.

[Qix-]

This is a bug, the destroy method is unnecessary and this bug shouldn't exist. v5 will have this fixed, but when v5 occurs is still up in the air as it's going to be a laborious release.

[ibc]

Thanks. Just wondering: how is this supposed to be fixed? AFAIU the leak happens because debug instances are stored in an array, and they are stored in an array to make it possible for debug.enable() and debug.disable() to affect existing debug instances. What is the solution for this without an explicit method called by the app?

[Qix-]

WeakMap. IE10 support ends this month, which means it's an opportune time to use it as everything LTS or newer right now supports it besides IE10.

[overflowz]

As far as I know, WeakMap is not iterable and only keys you can have there can be instances only. May I know how are you going to solve that problem with that? I am really curious though.

[ibc]

Exactly. WeakMap or WeakSet cannot be used for this. You don't have access to its content. You just can know whether they contain an object (a debug instance in this case) or not.

[Qix-]

You're right; a WeakSet could have been used to implement this if need be, but it turns out this isn't necessary.

I went ahead and proposed a fix in #740 because frankly I'm tired of people bringing this issue up, lol.

Please test it.