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

Any error during analysis will cause memory leak #15

Closed
SuicaLondon opened this issue Jan 27, 2024 · 6 comments
Closed

Any error during analysis will cause memory leak #15

SuicaLondon opened this issue Jan 27, 2024 · 6 comments

Comments

@SuicaLondon
Copy link
Contributor

This ticket is relative to this issue but it has different behaviour and causes memory leak.
EEC3A631-B6BA-41DC-9412-56F530C7001A

I am currently using a MacBook Pro 14 inch with M1 Pro with 16G Ram, OS version is 14.1

If the analysis process encounters an error, such as the one mentioned, there may not be an apparent issue. However, problems may arise when committing a large number of files simultaneously, like during a 300-file refactoring. In such cases, the process might exhaust the computer's memory and continually create new Dart processes in the background. Upon investigating the code, I observed that the process fails to terminate in the event of an error [will not kill the process](https://github.com/hyiso/lint_staged/blob/main/lib/s
Screenshot 2024-01-27 at 19 13 57
rc/run.dart).

What I suggest is that, add the kill process code on error handling as the Flutter does not have feature of Future.allSettled()

 if (result.stderr.toString().trim().isNotEmpty) {
          messsages.add(red(result.stderr.toString().trim()));
          Process.killPid(result.pid); // Kill process here
}
if (result.stdout.toString().trim().isNotEmpty) {
          messsages.add(result.stdout.toString().trim());
}
        _verbose(messsages.join('\n'));
 if (result.exitCode != 0) {
          ctx.output.add(messsages.join('\n'));
          ctx.errors.add(kTaskError);
          Process.killPid(result.pid); // Kill process here
 }

As I'm uncertain about the business logic, this may not work as the message does not update. So it seems the issue could be attributed to the process not terminating properly, causing it to stay in the memory forever.

@hyiso
Copy link
Owner

hyiso commented Mar 8, 2024

@SuicaLondon
Sorry for late reply, Could you help to check whether flutter/flutter#18225 (comment) can help?

@SuicaLondon
Copy link
Contributor Author

@hyiso Thank you fro reply. I have already checked this issue and tried before I opened this issue.
Screenshot 2024-03-11 at 23 31 08
I recently changed the code on my local and handle one big mr (about 300+ files). It doesn't run out of memory but it still will create multiple process of analysis.

In conclusion.

  1. This error is the only error that displayed on the terminal. Sometime the --verbose does not show any error while the memory is ran out.
  2. Added the Process.killPid did help the running out of memory issue, but it will still have multiple processes running on the background. The memory is still leaking for some reason. ( But the memory usage is definitely decreased.

I guess it may has multiple issues are happening at the same time. One is the pending executable is never ended, another is some how the memory does not released.

@hyiso
Copy link
Owner

hyiso commented Mar 12, 2024

@SuicaLondon
Thanks for the detailed information, the memory issue is ignored before, maybe a process pool is needed to limit the number of running process, I'll try it later.

@BTMuli
Copy link

BTMuli commented Apr 27, 2024

How is this problem solved now?It's easily crashed after commit over 100 files and it open 352 threads at one time——which cost 8020.2MB memory 😅

@BTMuli
Copy link

BTMuli commented Apr 27, 2024

A temporary solution: When there are multiple file changes that need to be committed, commit no more than 10 files at a time, and then convert them to one commit by git amend

@SuicaLondon
Copy link
Contributor Author

SuicaLondon commented May 14, 2024

As @BTMuli suggested, the problem of memory cost is caused by the dart analyze can only accept one file at the same time and it will create a new process. We can either wait for Dart to changed it or add a new parameter to limit the number of running processes by the number of the thread of the CPU.

dart run lint_staged --limit-process 

Then, we can implement a thread pool to manage that. If the project owner think it is okay, I can implement that and test it on my project and create an mr.

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

No branches or pull requests

3 participants