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

Let garbage collector to clean up native resources like file handles #38681

Closed
misos1 opened this issue May 13, 2021 · 3 comments
Closed

Let garbage collector to clean up native resources like file handles #38681

misos1 opened this issue May 13, 2021 · 3 comments

Comments

@misos1
Copy link

misos1 commented May 13, 2021

Is your feature request related to a problem? Please describe.

#38487 (comment)

For example when using fs.createReadStream and not using result then it is garbage collected but the file handle is not closed.

I understand that this cannot completely resolve EMFILE errors. But it could mitigate occasional file handle leaks which appear in many npm modules today - when it does not happen too frequently then GC could possibly catch them before it is too late. But without GC catching them they could build up for months and eventually cause a big EMFILE malfunction.

Describe the solution you'd like
For example streams could close file handle automatically (when they are garbage collected like in Python).

Describe alternatives you've considered
Maybe for example file streams should not open files until something is trying to read it?

@jasnell
Copy link
Member

jasnell commented May 13, 2021

This has been discussed and is not trivial. It is something that is being considered. I'm going to close this issue as a duplicate since there are existing issues (e.g. see #37874 (comment)) where this is already under discussion

/cc @addaleax

@jasnell jasnell closed this as completed May 13, 2021
@addaleax
Copy link
Member

In addition to that … if we do start up cleaning resource handles on GC, that’s going to mean that it’s less consequential if cleanup for resources is missed or wrong, which means that there’s a good chance that we would actually see more resource leaks in practice.

@misos1
Copy link
Author

misos1 commented May 13, 2021

This has been discussed and is not trivial. It is something that is being considered.

It is probably silliness but maybe could be used something like FinalizationRegistry?

In addition to that … if we do start up cleaning resource handles on GC, that’s going to mean that it’s less consequential if cleanup for resources is missed or wrong, which means that there’s a good chance that we would actually see more resource leaks in practice.

I think this is debatable.

Actually people are probably not really used to manually closing streams because they normally do not need to - for example fs.createReadStream will close a file when it is fully read (when used with pipeline it will correctly handle destruction on errors).

And probably there are many people who do not expect that when such stream is not used then not everything will be released. I think there should be more words about closing streams and leaks in documentation (I can see just 2 occurrences when I search for word "leak" on https://nodejs.org/api/stream.html).

There are languages like C where everything must be closed or deallocated manually and people know this but they still make mistakes (and probably make more leaks than people in javascript running in nodejs).

It could be rather hard to see consequences if cleanup for resources is missed or wrong. For example on Ubuntu 20.04 the default max open files is about one million so it could take a really long time until EMFILE happens. If nodejs would be able to close files on GC then it would be able to print warnings about this (possibly if enabled by switch) - it may treat such things as bad practice (this could be usable even without actually closing file handles).

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