-
Notifications
You must be signed in to change notification settings - Fork 936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak when creating debug instances #740
Conversation
It works fine :) Will you release this in v4? |
Yes, this would be a minor release if pushed. |
"If pushed"? :) What does it depend on? I've already changed my projects 3 times today to conform to this issue :) |
Making sure I didn't make a mistake and break millions of users. Don't expect this to be released in the coming week. For comparison, this is roughly 10x more than React. You can replace your |
Sure, thanks. |
I have encountered this problem in the production environment. So I sincerely hope this can be merged ASAP. |
Which is the state of this PR? is it expected to be merged soon? I already tested it and works fine, I understand it's not trivial to move this into production release but, what is the current state or plan? |
Thanks, somehow I already see it's still open :) |
Just another follow up that the no-leak branch solved some memory issues I was seeing with a project. |
Another follow up - This branch has just fixed the memory leak that's been haunting me for the past 6 months |
Published as |
Fixing memory leaks is of course important, but I wonder whether this merge is a performance regression. Has it been tested? |
@ab-pm You're exactly right, good eye. It's a well known issue that I've been thinking about since I started maintaining this library. v5 is going to be a rewrite that will improve this a bit.
As for whether or not it's a "performance regression", by this logic anything that adds an extra instruction to the overall code path is a "performance regression". The trade-off here is that The real fix here would be to overhaul the enabled namespaces system altogether, which would be a major breaking change and is thorougly discussed elsewhere on the issue tracker. As I mentioned before, it's slated for v5, whenever I get around to that. |
@qix Well it's not just "an extra instruction", but quite many of them, on what might be a hot code path :-) But if you've consciously made the trade-off to favour memory safety, I'm totally fine with this - that's all I wanted to know, thanks! |
Would a backport of this memory fix leak to 3.x.x version be possible? Some packages do not want to upgrade to debug:4.x in order to not lose compatibility with earlier-but-apparently-still-used versions of Node.js... |
Closes #678 .
One possible solution to the memory leak and needless
.destroy()
method.Will leave open for a while to allow people to test.