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

doc: fix filehandle.truncate sample codes #20913

Closed

Conversation

shisama
Copy link
Contributor

@shisama shisama commented May 23, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels May 23, 2018
doc/api/fs.md Outdated
const fd = await fsPromises.open('temp.txt', 'r+');
await fsPromises.ftruncate(fd, 4);
const filehandle = await fsPromises.open('temp.txt', 'r+');
await filehandle.truncate(4);
console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints: Node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should really be....

let filehandle;
try {
  const filehandle = await fsPromises.open('temp.txt', 'r+');
  await filehandle.truncate(4);
} finally {
  await filehandle.close();
}

While the filehandle will close automatically on garbage collection in order to prevent the leak, it should be closed manually. A process warning would be emitted if it is allowed to close on gc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell sorry for the tangent: we really need a better resources story in general - if this is tricky consider how hard this is with 3 concurrent handles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that might work here API wise is a disposer:

await fsPromises.open('temp.txt', 'r+', async handle => {
  await handle.truncate(4);
});

This doesn't solve the harder problem of dealing with multiple resources at once though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly don't disagree :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell
Thank you for your comment. The codes were fixed with filehandle.close. Please check them again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell - what do you think the right avenue to discuss such a cross-cutting concern is? I'm not sure what the right team/working-group/avenue is.

I'm a little lost here :)

Fix sample codes because `filehandle.close` was not called when error
occured. `filehandle.close` should be called manually.
@benjamingr
Copy link
Member

Collaborators - please 👍 if you agree with fast-tracking this.

doc/api/fs.md Outdated
filehandle = await fsPromises.open('temp.txt', 'r+');
await filehandle.truncate(4);
} finally {
if (filehandle !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can make undefined filehandle be null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The open throwing, since that was unclear it'll definitely be unclear for some users - we should probably add a comment.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If open throws, will not the filehandle remain undefined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have sworn that was a != and not a !==, good spotting!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, shoud not we initialize filehandle as null or use non-strict comparison or even if (filehandle) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should. I was saying "good spotting" for spotting that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, GitHub did not update timely with new comments)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsemozhetbyt @benjamingr
filehandle is not undefined. It is defined and initialized as null.

let filehandle;

But it is unclear for some users.
The below code may be more clearly for users. It looks better?

async function doTruncate() {
  let filehandle = null;
  try {
    filehandle = await fsPromises.open('temp.txt', 'r+');
    await filehandle.truncate(4);
  } finally {
    if (filehandle) {
      // close the file if it is opened.
      await filehandle.close();
    }
  }
  console.log(fs.readFileSync('temp.txt', 'utf8'));  // Prints: Node
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let filehandle;

When you do that in JavaScript that initializes the value to undefined. You can verify this in the following script:

let filehandle;
console.log(filehandle);

The below code may be more clearly for users. It looks better?

Yes, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr
oh, sorry! I was wrong and fixed them.

doc/api/fs.md Outdated
console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints Node.js\0\0\0
}

doTruncate().catch(console.error);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unneeded empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsemozhetbyt Fixed it. Thanks.

shisama added 2 commits May 24, 2018 13:35
Fix sample codes for filehandle.truncate to make them more clearly.
@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label May 24, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 24, 2018

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/749/ (one unrelated fail of a flaky test).

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2018
@Trott
Copy link
Member

Trott commented May 24, 2018

vsemozhetbyt pushed a commit that referenced this pull request May 24, 2018
PR-URL: #20913
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in ac82261
Thank you!

targos pushed a commit that referenced this pull request May 25, 2018
PR-URL: #20913
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants