hyiso / lint_staged

Run linters on git staged files for your Flutter and Dart projects. Inspired by JavaScript lint-staged library
https://pub.dev/packages/lint_staged
Apache License 2.0
8 stars 2 forks source link

Any error during analysis will cause memory leak #15

Closed SuicaLondon closed 3 months ago

SuicaLondon commented 8 months ago

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 commented 6 months ago

@SuicaLondon Sorry for late reply, Could you help to check whether https://github.com/flutter/flutter/issues/18225#issuecomment-396315283 can help?

SuicaLondon commented 6 months ago

@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 commented 6 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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.