doc: fix filehandle.truncate sample codes#20913
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added doc 
| awaitfsPromises.ftruncate(fd, 4); | ||
| constfilehandle=awaitfsPromises.open('temp.txt', 'r+'); | ||
| awaitfilehandle.truncate(4); | ||
| console.log(fs.readFileSync('temp.txt', 'utf8')); // Prints: Node |
There was a problem hiding this comment.
these should really be....
letfilehandle;try{constfilehandle=awaitfsPromises.open('temp.txt','r+');awaitfilehandle.truncate(4);}finally{awaitfilehandle.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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
One thing that might work here API wise is a disposer:
awaitfsPromises.open('temp.txt','r+',asynchandle=>{awaithandle.truncate(4);});This doesn't solve the harder problem of dealing with multiple resources at once though.
There was a problem hiding this comment.
@jasnell
Thank you for your comment. The codes were fixed with filehandle.close. Please check them again.
There was a problem hiding this comment.
@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 commented May 23, 2018
Collaborators - please 👍 if you agree with fast-tracking this. |
doc/api/fs.md Outdated
| filehandle =awaitfsPromises.open('temp.txt', 'r+'); | ||
| awaitfilehandle.truncate(4); | ||
| } finally{ | ||
| if (filehandle !==null){ |
There was a problem hiding this comment.
What can make undefined filehandle be null?
There was a problem hiding this comment.
The open throwing, since that was unclear it'll definitely be unclear for some users - we should probably add a comment.
There was a problem hiding this comment.
If open throws, will not the filehandle remain undefined?
There was a problem hiding this comment.
I could have sworn that was a != and not a !==, good spotting!
There was a problem hiding this comment.
I mean, shoud not we initialize filehandle as null or use non-strict comparison or even if (filehandle) ?
There was a problem hiding this comment.
We should. I was saying "good spotting" for spotting that.
There was a problem hiding this comment.
Sorry, GitHub did not update timely with new comments)
There was a problem hiding this comment.
@vsemozhetbyt@benjamingrfilehandle is not undefined. It is defined and initialized as null.
letfilehandle;But it is unclear for some users.
The below code may be more clearly for users. It looks better?
asyncfunctiondoTruncate(){letfilehandle=null;try{filehandle=awaitfsPromises.open('temp.txt','r+');awaitfilehandle.truncate(4);}finally{if(filehandle){// close the file if it is opened.awaitfilehandle.close();}}console.log(fs.readFileSync('temp.txt','utf8'));// Prints: Node}There was a problem hiding this comment.
let filehandle; When you do that in JavaScript that initializes the value to undefined. You can verify this in the following script:
letfilehandle;console.log(filehandle);The below code may be more clearly for users. It looks better?
Yes, thank you.
There was a problem hiding this comment.
@benjamingr
oh, sorry! I was wrong and fixed them.
doc/api/fs.md Outdated
| } | ||
| doTruncate().catch(console.error); | ||
There was a problem hiding this comment.
Nit: unneeded empty line.
Fix sample codes for filehandle.truncate to make them more clearly.
vsemozhetbyt added the fast-track 
CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/749/ (one unrelated fail of a flaky test). |
vsemozhetbyt added the author ready 
Trott commented 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 commented May 24, 2018
Landed in ac82261 |
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]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes