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

fsPromises.truncate doesn't close fd. #34189

Closed
sheepa opened this issue Jul 4, 2020 · 8 comments
Closed

fsPromises.truncate doesn't close fd. #34189

sheepa opened this issue Jul 4, 2020 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@sheepa
Copy link

sheepa commented Jul 4, 2020

What steps will reproduce the bug?

fsPromises.truncate(path) will result in a warning a few seconds later: (node:1387179) Warning: Closing file descriptor 22 on garbage collection - Using the callback truncate await new Promise((res, rej) => { fs.truncate(file, (err, ret) => { if(err) rej(err); else res(ret) }) }) works fine without such warning.

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

Not having this warning.

What do you see instead?

Additional information

@himself65
Copy link
Member

Could you show the code to reproduce?

@himself65 himself65 added the fs Issues and PRs related to the fs subsystem / file system. label Jul 4, 2020
@bnoordhuis
Copy link
Member

I'm not able to reproduce. As an example:

$ node --expose_gc
> fs.promises.truncate('/tmp/x.txt').then(_ => gc())  // x.txt exists
Promise { <pending> }
> 

No warnings.

@sheepa I'm going to close this but I can reopen if you post steps to reproduce.

@bnoordhuis bnoordhuis added the invalid Issues and PRs that are invalid. label Jul 7, 2020
@sheepa
Copy link
Author

sheepa commented Jul 7, 2020

const fs = require('fs').promises

async function test()
{
  const file = '/tmp/x.txt'
  await fs.writeFile(file, 'data')
  await fs.truncate(file)
  gc()
}

test()

The following code give me (node:268029) Warning: Closing file descriptor 19 on garbage collection

@jasnell jasnell reopened this Jul 7, 2020
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@bnoordhuis... closing this was premature.

This is an issue that was fixed in master in eadc385 but it does not look like the fix was backported to 12.x yet. See... https://github.com/nodejs/node/blob/v12.x/lib/internal/fs/promises.js#L307-L309

C:\Users\jasne\Projects\tmp>node --expose-gc
Welcome to Node.js v12.18.0.
Type ".help" for more information.
> fs.promises.truncate('t.js').then(_ => gc())
Promise { <pending> }
> (node:23652) Warning: Closing file descriptor 4 on garbage collection

@jasnell jasnell added confirmed-bug Issues with confirmed bugs. v12.x and removed invalid Issues and PRs that are invalid. labels Jul 7, 2020
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@sheepa ... I'll be opening a PR to backport the fix to 12.x soon but it will take a while for that to land in a release. You can work around the issue for now by using the ftruncate() method instead using the exact same pattern used in the fix:

async function myTruncate(path, len) {
  const fd = await open(path, 'r+');
  return ftruncate(fd, len).finally(fd.close.bind(fd));
}

@richardlau
Copy link
Member

This is an issue that was fixed in master in eadc385 but it does not look like the fix was backported to 12.x yet. See... https://github.com/nodejs/node/blob/v12.x/lib/internal/fs/promises.js#L307-L309

eadc385 references #28858 as the PR and that's marked semver-major so it will be ignored by our tooling (e.g. branch-diff) and processes when evaluating what could be backported.

@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@richardlau ... yeah, I spotted that. This one commit is not semver-major. I'm working on a backport PR now for 12.x but github is giving me the Unicorn of Sadness at the moment. Will open as soon as the site becomes responsive again

@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

PR opened!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

6 participants