Skip to content
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 in the Scheduler #5

Closed
wants to merge 1 commit into from

Conversation

staltz
Copy link

@staltz staltz commented Dec 7, 2020

Hi! Thanks for this module, our team began using it and it seems good.

This PR is also a sort of report of an issue. We detected a memory leak. If writeFile is called many times synchronously, it keeps piling up the queue in the scheduler, and because of this length > 1 if condition, when length is a large number, it just returns and allows the queue to keep growing. This led to a memory leak that eventually consumed 100% of the computer's memory, grinding to a full stop.

The proposed change is not necessarily an elegant one, but I've found it to work to prevent the leak. Feel free to not accept this patch and instead find a more elegant solution. In fact, I tried to understand this file, but it seemed hard to understand, so I'll leave the details to you.

Thanks

@fabiospampinato
Copy link
Owner

This isn't really a PR I can merge, it's a magic number that breaks something else without really fixing the root issue anyway.

I think what might be happening is that one of the tasks being scheduled throws an error somewhere, so the queue halts forever essentially.

It's a little strange that you saw an out of memory error because of this though 🤔 are you writing to the same file a million times or something? Did you raise the default timeouts massively?

It'd be useful if you could reproduce the issue reliably and share the code, so that we can spot where the root issue happens. In any case I'll look into this, thanks for reporting the issue 👍.

@staltz
Copy link
Author

staltz commented Dec 7, 2020

you writing to the same file a million times or something?

Yes. To clarify, we solved the issue by (obviously) not writing to the same file synchronously millions of times. But it would be good if atomically didn't have a memleak even if we kept that.

@fabiospampinato
Copy link
Owner

fabiospampinato commented Dec 7, 2020

But it would be good if atomically didn't have a memleak even if we kept that.

This isn't really a memory leak, the library queues writes, you are telling it to do millions of writes, it must keep the data in memory somewhere, the issue is queuing millions of writes to being with.

I've taken a closer look at the codebase, and I don't believe the library can throw an error that would cause queues to stop being processed, if that's not true I'm going to need to see some code proving me wrong.

There's this potential optimization that as a side effect would fix this "issue", I think it probably is a good idea to implement that for other reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants