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

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

This comment has been minimized.

Copy link
@jasnell

jasnell May 23, 2018

Member

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.

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

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

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

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.

This comment has been minimized.

Copy link
@jasnell

jasnell May 23, 2018

Member

I certainly don't disagree :-)

This comment has been minimized.

Copy link
@shisama

shisama May 23, 2018

Author

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

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

@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

This comment has been minimized.

Copy link
Member

benjamingr commented May 23, 2018

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

filehandle = await fsPromises.open('temp.txt', 'r+');
await filehandle.truncate(4);
} finally {
if (filehandle !== null) {

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt May 23, 2018

Contributor

What can make undefined filehandle be null?

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt May 23, 2018

Contributor

If open throws, will not the filehandle remain undefined?

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt May 23, 2018

Contributor

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

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 23, 2018

Member

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt May 23, 2018

Contributor

Sorry, GitHub did not update timely with new comments)

This comment has been minimized.

Copy link
@shisama

shisama May 24, 2018

Author

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

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 24, 2018

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

This comment has been minimized.

Copy link
@shisama

shisama May 24, 2018

Author

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

console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints Node.js\0\0\0
}
doTruncate().catch(console.error);

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt May 23, 2018

Contributor

Nit: unneeded empty line.

This comment has been minimized.

Copy link
@shisama

shisama May 24, 2018

Author

@vsemozhetbyt Fixed it. Thanks.

shisama added 2 commits May 24, 2018
Fix sample codes for filehandle.truncate to make them more clearly.
@vsemozhetbyt

This comment has been minimized.

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

@Trott
Trott approved these changes May 24, 2018
@Trott

This comment has been minimized.

Copy link
Member

Trott commented May 24, 2018

vsemozhetbyt added a commit that referenced this pull request May 24, 2018
PR-URL: #20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

vsemozhetbyt commented May 24, 2018

Landed in ac82261
Thank you!

targos added a commit that referenced this pull request May 25, 2018
PR-URL: #20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
shisama pushed a commit to shisama/node that referenced this pull request May 30, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request May 30, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request Jun 5, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request Jun 9, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request Jun 11, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request Jun 12, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
shisama pushed a commit to shisama/node that referenced this pull request Jun 17, 2018
PR-URL: nodejs#20913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.