Skip to content

Conversation

@ronag
Copy link
Member

Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Calling close on a file description which is currently in use is undefined behavior due to implementation details in libuv. Add a guard against this when using FileHandle.
@ronagronag requested review from addaleax and jasnellAugust 12, 2020 10:32
@nodejs-github-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 12, 2020
@ronagronagforce-pushed the safe-file-handle branch 8 times, most recently from 03ee3d5 to bc670bdCompareAugust 12, 2020 11:11
@ronagronagforce-pushed the safe-file-handle branch 2 times, most recently from 6169687 to 107f508CompareAugust 12, 2020 11:15
@ronagronagforce-pushed the safe-file-handle branch 4 times, most recently from 4a3099a to 711653aCompareAugust 12, 2020 21:05
@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
MemberAuthor

@nodejs/fs

@jasnell
Copy link
Member

jasnell commented Aug 12, 2020

@addaleax ... This brought an edge case to mind that I'm not sure how we're handling...

What should happen in the following case

'use strict';constfs=require('fs');const{ Worker, workerData, isMainThread }=require('worker_threads');if(isMainThread){asyncfunctionfoo(){constfh=awaitfs.promises.open(__filename);constbuffer=Buffer.alloc(1000);// Not awaiting...constp=fh.read(buffer,0,1000);// Transferring the FileHandle while there's potentially// an outstanding async operation on it.constw=newWorker(__filename,{workerData: fh,transferList: [fh]});// Then awaiting...awaitp;returnbuffer.toString();}foo().then(console.log);}else{workerData.close();}

First immediate thought is that we should throw if attempting to transfer a FileHandle that has pending operations.

@addaleax
Copy link
Member

First immediate thought is that we should throw if attempting to transfer a FileHandle that has pending operations.

Yeah, I agree – that sounds like the best approach to me 👍

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

It would likely be good to add a comment to the fs.md documentation for FileHandle.close() that pending operations will be completed.

@ronag
Copy link
MemberAuthor

It would likely be good to add a comment to the fs.md documentation for FileHandle.close() that pending operations will be completed.

Fixed

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 13, 2020

addaleax added a commit to addaleax/node that referenced this pull request Aug 13, 2020
This can currently be triggered when posting a closing FileHandle. Refs: nodejs#34746 (comment)
@ronag
Copy link
MemberAuthor

Landed in a0f87aa

@ronagronag closed this Aug 14, 2020
ronag added a commit that referenced this pull request Aug 14, 2020
Calling close on a file description which is currently in use is undefined behavior due to implementation details in libuv. Add a guard against this when using FileHandle. PR-URL: #34746 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Aug 17, 2020
This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Aug 17, 2020
This can currently be triggered when posting a closing FileHandle. Refs: nodejs#34746 (comment) PR-URL: nodejs#34766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
Calling close on a file description which is currently in use is undefined behavior due to implementation details in libuv. Add a guard against this when using FileHandle. PR-URL: #34746 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Aug 18, 2020
This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Backport-PR-URL: #34814 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
Calling close on a file description which is currently in use is undefined behavior due to implementation details in libuv. Add a guard against this when using FileHandle. PR-URL: #34746 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Backport-PR-URL: #34814 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 27, 2020
This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Backport-PR-URL: #34814 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 28, 2020
This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Backport-PR-URL: #34814 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ronag@nodejs-github-bot@jasnell@addaleax@devnexen