fs: move FileHandle close on GC to EOL #58536
Conversation
Automatically closing a FileHandle on garbage collection has been deprecated for some time now. We've said that it will eventually be removed and become and error.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58536 +/- ##
==========================================
+ Coverage 90.21% 90.25% +0.03%
==========================================
Files 635 635
Lines 187580 187575 -5
Branches 36853 36859 +6
==========================================
+ Hits 169231 169290 +59
+ Misses 11108 11055 -53
+ Partials 7241 7230 -11
🚀 New features to boost your workflow:
|
Unless there already is a trivial userspace solution to this or |
There was a problem hiding this comment.
Sorry, I have to be more explicit here.
I don't think we should land a breaking change at the same time as the feature that allows to avoid breakage in userland.
The autoClose option must be a separate, backportable,
semver-minor
autoClose.
Also I think that in the
semver-major
autoClose should become true by default, to align with filehandle.createReadStream()/filehandle.createWriteStream()/etc. and to minimize breakage.
There are a few test failures.
fs-read-offset-null and fs-read-empty-buffer look trivial to fix (#58543)
The FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope in https://ci.nodejs.org/job/node-test-commit-arm-debug/18769/ also might be related.
| try { | ||
| const hostname = await fileHandle.readFile(); | ||
| assert.ok(hostname.length > 0); | ||
| } finally { | ||
| await fileHandle.close(); | ||
| } |
There was a problem hiding this comment.
Nit: since this is part of semver-major change, we can use ERM right away.
| try { | |
| const hostname = await fileHandle.readFile(); | |
| assert.ok(hostname.length > 0); | |
| } finally { | |
| await fileHandle.close(); | |
| } | |
| { | |
| await using fileHandle = await open('/proc/sys/kernel/hostname', 'r'); | |
| const hostname = await fileHandle.readFile(); | |
| assert.ok(hostname.length > 0); | |
| } |
| const ondone = FunctionPrototypeBind(this[kUnref], this); | ||
| const ondone = async () => { | ||
| this[kUnref](); | ||
| if (options.autoClose) this.close(); |
There was a problem hiding this comment.
Shouldn't we await here?
| if (options.autoClose) this.close(); | |
| if (options.autoClose) await this.close(); |
| // throwing the error until the next immediate queue tick so as not | ||
| // to interfer with the gc process. | ||
| env()->SetImmediate([](Environment* env) { | ||
| THROW_ERR_INVALID_STATE(env, |
There was a problem hiding this comment.
Lint
| THROW_ERR_INVALID_STATE(env, | |
| THROW_ERR_INVALID_STATE( | |
| env, |
Automatically closing a FileHandle on garbage collection has been deprecated for some time now. We've said that it will eventually be removed and become and error.
This PR also adds a new
autoCloseoption to theFileHandlereadableWebStream()API in order to deal with a possible bug in which the readable stream that wraps theFileHandleis passed out of a context where theFileHandleis accessible to be closed.