-
Notifications
You must be signed in to change notification settings - Fork 86
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: fix the memory leak introduced in nodejs v10 patch #303
Conversation
Looking into this now. First results were inconclusive. It looked like it was working at first, but one of the threads still climbs over 1gb, which doesn't seem right to me. So, I'm doing some memory profiling now. Going to examine heap snapshots. Will let you know! |
Looks good! I pre-loaded it with one large file (149,382,687 bytes) This is the heap comparison of before -> after For good measure, I also mocked & restored repeatedly with the same data about 6 times in a row. Reported memory use in task manager stayed the same. Thanks for the fast work! BTW, Have you had a chance to look at my PR? |
@tschaub can we merge this? |
@3cp - yes please. Are you able to? Every time I get the notification, I’m not on a device that is allowed to merge with the failed test. |
I cannot. Tried to skip win32 node 12 in CI, but GitHub is not happy about it, maybe I missed some config. |
Thanks for the fix, @3cp. Sorry it took so long to get in. |
Can we get a release 🙏 ? |
closes #302