Skip to content

doc: fix filehandle.truncate sample codes #20913

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.

Sign up for GitHub

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

Closed
shisama wants to merge 4 commits into from
Closed

doc: fix filehandle.truncate sample codes #20913

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32107d0
doc: fix filehandle.truncate sample codes
May 23, 2018
89fc020
doc: fix filehandle.truncate sample codes to call filehandle.close
May 23, 2018
dda739e
doc: remove unneeded empty line in fs.md
May 24, 2018
01a67c1
doc: fix sample codes for filehandle.truncate
May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3572,8 +3572,15 @@ console.log(fs.readFileSync('temp.txt', 'utf8'));
// Prints: Node.js

async function doTruncate() {
const fd = await fsPromises.open('temp.txt', 'r+');
await fsPromises.ftruncate(fd, 4);
let filehandle;
try {
filehandle = await fsPromises.open('temp.txt', 'r+');
await filehandle.truncate(4);
} finally {
if (filehandle !== null) {
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.

What can make undefined filehandle be null?

Copy link
Member

@benjamingr benjamingr May 23, 2018

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 •
edited
Loading

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

@benjamingr benjamingr May 23, 2018

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

@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.

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

Copy link
Member

@benjamingr benjamingr May 23, 2018

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

@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.

Sorry, GitHub did not update timely with new comments)

Copy link
Contributor Author

@shisama shisama May 24, 2018

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

@benjamingr benjamingr May 24, 2018

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

@shisama shisama May 24, 2018

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.

await filehandle.close();
}
}
console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints: Node
Copy link
Member

@jasnell jasnell May 23, 2018

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

@benjamingr benjamingr May 23, 2018

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

@benjamingr benjamingr May 23, 2018

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

@jasnell jasnell May 23, 2018

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

@shisama shisama May 23, 2018

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

@benjamingr benjamingr May 23, 2018

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 :)

}

Expand All @@ -3591,8 +3598,15 @@ console.log(fs.readFileSync('temp.txt', 'utf8'));
// Prints: Node.js

async function doTruncate() {
const fd = await fsPromises.open('temp.txt', 'r+');
await fsPromises.ftruncate(fd, 10);
let filehandle;
try {
filehandle = await fsPromises.open('temp.txt', 'r+');
await filehandle.truncate(10);
} finally {
if (filehandle !== null) {
await filehandle.close();
}
}
console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints Node.js\0\0\0
}

Expand Down