[WIP] fs: add FD object to manage file descriptors #18109

jasnell · 2018-01-11T22:11:46Z

In preparation for providinng a promisified fs API, introduce a FD wrapper class that automatically closes
the file descriptor on garbage collection, helping to ensure that the fd will not leak.

The promisified fs API will use these objects instead of the numeric file descriptors in order to ensure that fd's are closed appropriately following promise resolve or reject operations.

This was extracted from the WIP fs promises PR: #17739

/cc @addaleax @mcollina

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

Affected core subsystem(s)

fs

jasnell · 2018-01-11T22:18:29Z

Note: None of the existing fs APIs are modified to use this new object. To use the new object with, for instance, fs.fstat() or fs.fstatSync(), use the fd property on the object... e.g. fs.fstatSync(fd.fd).

jasnell · 2018-01-11T22:20:50Z

src/node_file.cc

Take note: this uses a sync close. If we decide that it's necessary to use an async close, we can switch to that.

There is an open question about what to do with the result. We can use the new env->SetImmediate() to schedule a callback out to javascript land after the close attempt in order to report any errors or to give the equivalent to fs.close(fd, callback), but given that the primary use case is for this to close on garbage collection, I did not consider adding that mechanism to be critical.

If there is an error, will the file descriptor be leaked? Can we assure that we have no leak before swallowing?
This mechanism implies that we are cleaning up after ourselves whatever happens.

IMHO it should go to ‘uncaughtException’ if it is failing, and he OS fd is still there.

Trott · 2018-01-11T22:29:57Z

doc/api/fs.md

Should this be fd.close()?

addaleax · 2018-01-11T22:51:44Z

Tbh, I don’t like the idea of exposing this as a public API.

The JS engine is not aware about file descriptors being a (comparatively) scarce resource, so the garbage collector might not act on the need for cleaning these up; closing the FD once its usage is finished is still the right thing to do.

I would definitely approve of this feature being enabled for fs streams, though – in those cases, we already do our best to close the FD as early as we can.

vsemozhetbyt · 2018-01-11T22:42:41Z

doc/api/fs.md

Should this be something like {[fs.FD][#fs_class_fs_fd]} or {[fs.FD][]} with a bottom reference?

vsemozhetbyt · 2018-01-11T22:52:08Z

doc/api/fs.md

fs.open() -> fs.openFD()

vsemozhetbyt · 2018-01-11T22:53:29Z

doc/api/fs.md

fs.openSync() -> fs.openFDSync()

vsemozhetbyt · 2018-01-11T22:56:45Z

doc/api/fs.md

fs.open() -> fs.openFD()

vsemozhetbyt · 2018-01-11T22:58:15Z

doc/api/fs.md

fs.open( -> fs.openFD(

vsemozhetbyt · 2018-01-11T22:58:37Z

doc/api/fs.md

fs.open( -> fs.openFD(

vsemozhetbyt · 2018-01-11T23:02:49Z

doc/api/fs.md

fs.open() -> fs.openFD()? Is this note still applicable here?

vsemozhetbyt · 2018-01-11T23:04:28Z

doc/api/fs.md

fs.open() -> fs.openFD()?

vsemozhetbyt · 2018-01-11T23:05:39Z

doc/api/fs.md

The same as above?

vsemozhetbyt · 2018-01-11T23:07:02Z

doc/api/fs.md

fs.open() -> fs.openFD() with a new link?

jasnell · 2018-01-11T23:25:09Z

@addaleax ... I tend towards the opposite direction... that this is what the public API should have been from the beginning. @mcollina ... thoughts?

mcollina · 2018-01-12T07:57:54Z

I debugged too many leaked file descriptor situations because Node did not have this feature.

I think fd manipulation is somehing very few are doing directly. For those that want to do that, this would simplify things considerably. For everybody else, it changes nothing. I’m +1.

Can you also use it within streams in a separate commit in the PR? I think it will help making a case for it.

Can we add something similar for net as well? Can it be made generic?

addaleax · 2018-01-12T08:10:31Z

Can you also use it within streams in a separate commit in the PR? I think it will help making a case for it.

Can we add something similar for net as well? Can it be made generic?

I’ve thought about these quite a bit in the past, and the thing is, that does not work (for readable streams). These objects need to be GC roots, because unfortunately there is no reason why any 'data' listeners would need to retain backreferences to the file stream/socket/etc..

For example, this code is perfectly valid (and common) releasing the stream for garbage collection would break it – and worst of all, in a non-deterministic way:

fs.createReadStream('file.txt')
  .on('data', ondata).on('end', end);

that this is what the public API should have been from the beginning

If that is the goal here, I would propose:

Calling the class File or something like that – we don’t need to retain the low-level, UNIX-specific name here
Adding methods on it that correspond to the fs.foo(fd, …) functions, in a way that makes sure GC isn’t triggered before the call finishes

I think I could get on board with that.

mcollina · 2018-01-12T09:11:06Z

These objects need to be GC roots, because unfortunately there is no reason why any 'data' listeners would need to retain backreferences to the file stream/socket/etc..

@addaleax the problem for file descriptor leaking in streams comes when you do a.pipe(b) and a or b emit an 'error'. a and b could be garbage collected at that point, but the handle is leaked. That's what https://www.npmjs.com/package/pump address (it calls destroy()).

I think the stream reference is kept alive by the closure created within _read() (https://github.com/nodejs/node/blob/master/lib/fs.js#L2334-L2349) in the case you mentioned. The stream will not be collectable until it's exhausted.

addaleax · 2018-01-12T09:31:16Z

I think the stream reference is kept alive by the closure created within _read() (https://github.com/nodejs/node/blob/master/lib/fs.js#L2334-L2349) in the case you mentioned. The stream will not be collectable until it's exhausted.

There isn’t always a _read() call in progress, though.

mcollina · 2018-01-12T09:35:02Z

There isn’t always a _read() call in progress, though.

In the example you made, with on('data', something), there is always a call to _read() in progress (this.push(buf) schedules a _read()). The only case when that is not happening if someone use .pause() or .pipe(), in which case they will hold a reference to the stream.

In any case, if there is not a read scheduled and no one is holding a reference, then a file descriptor will be leaked.

bnoordhuis · 2018-01-12T11:30:35Z

Garbage collection is non-deterministic. Basing deterministic resource management on something that isn't is a terrible idea of the you-don't-need-this-even-if-you-think-you-do variety.

If the streams API has problems, they should be fixed, but not like this.

mcollina · 2018-01-12T12:31:40Z

Garbage collection is non-deterministic. Basing deterministic resource management on something that isn't is a terrible idea of the you-don't-need-this-even-if-you-think-you-do variety.

This is not about basing deterministic resource management on gc. This is about preventing silent file descriptor leaks.
I'm 👍 if we want to emit a warning in those cases where have to clean up on gc(). However, the current behavior (silently ignoring the leak) is not user-friendly.

bnoordhuis · 2018-01-12T12:43:21Z

A tool for detecting file descriptor leaks is a worthy goal but we can do much better than this PR. Just of the top off my head:

Instrument / monkey-patch functions that open and close file descriptors.
Record the call stack and the async_context id.
Add a function to retrieve that info again for currently open file descriptors.
Print it at intervals or on program exit.

Best of all: can be done as a third-party module, doesn't need to be built-in.

jasnell · 2018-01-12T16:04:16Z

@addaleax said:

Calling the class File or something like that

There is already a defined Web API for a File object and it's easily conceivable that we could provide an implementation of it someday. I'd rather not introduce a separate File object that implements a different API.

Adding methods on it that correspond to the fs.foo(fd, …) functions

We could but I'm not a huge fan of that. The existing fs API, for all of it's warts, is familiar and well established. The promises based version should really try not to diverge from it too much in order to make code migration less painful. Eventually we might get there, and this would give us a foundation to work from, but I'd generally prefer not. What I can do right now in order to make that easier, however, is have the public API expose a pure JS object with the internal native FD object used as an internal handle, which would make adding those methods later on much easier.

@bnoordhuis said:

. Basing deterministic resource management on something that isn't is a terrible idea of the you-don't-need-this-even-if-you-think-you-do variety.

As @mcollina indicates, this is not about replacing or eliminating explicitly calling close() and relying on gc, it's about providing a more robust fallback because we know for a fact that users are doing it wrong. We also know that the introduction of Promises with their Oh So Helpful Error Swallowing is only going to help make the issue harder to detect.

I like the idea of emitting a warning if the FD has to close on cleanup, and the documentation should likely deemphasize the "Hey, this will clean up for you on garbage collection" part in order to avoid encouraging users to do the wrong thing.

FWIW, I knew this bit was going to be controversial which is why I separated it out into it's own PR. There's no need to rush this part and I'd like to get it right.

addaleax · 2018-01-12T19:19:38Z

Calling the class File or something like that

There is already a defined Web API for a File object and it's easily conceivable that we could provide an implementation of it someday. I'd rather not introduce a separate File object that implements a different API.

Okay, fair point – my line of thinking was, file descriptors are not necessarily a concept that JS programmers are familiar with, so we might want to provide a more explicit identifier than FD.

The promises based version should really try not to diverge from it too much in order to make code migration less painful. Eventually we might get there, and this would give us a foundation to work from, but I'd generally prefer not.

We could make the methods on the new class Promises-only. 😉 But I’m also okay with saying “this is userland material” for that.

jasnell · 2018-01-12T22:18:16Z

Ok, so here's what I'm looking at for changes to this:

fd.close() will return a Promise that will resolve/reject when closing the fd is complete. In this case, the object will be prevented from garbage collecting until the promise is resolved/rejected.
If the user never calls fd.close() and the fd is closed on gc, a process warning will be emitted.
If, for whatever reason, fs.close() fails an unhandled error will be emitted. This can happen, for instance, if the fd num is closed using fs.close()/fs.closeSync().

How does that look?

bnoordhuis · 2018-01-12T23:23:05Z

That, to me, sounds pretty reasonable.

mcollina · 2018-01-12T23:37:00Z

Will adding a dual callback/promise system for fs.close() be problematic?

jasnell · 2018-01-13T16:50:34Z

@mcollina ... there won't be a dual callback. The FD object will have it's own close() method that will return a Promise. The existing fs.close() will not be touched and would not work with FD objects.

jasnell · 2018-01-15T21:13:15Z

Ok, I have points 1 and 2 implemented. I do not yet have point 3 implemented (failing with an unhandled error if closing during garbage collection). I will work on that next, but please take a look in the meantime

mcollina · 2018-01-17T12:23:55Z

doc/api/fs.md

should this return a Promise instead of returning a callback? the rest of the code is promise-based. It should be either 100% callbacks or 100% promises, having a callback here but fd.close().then() is not consistent.

Yeah, I'm going back and forth on this and thinking that rather than introduce fs.openFD() and fs.openFDSync() we just introduce a Promise returning fs.promises.openFD() and keep this isolated to use of the Promises API for now.

And if that is the approach to take on this, then I'll just close this PR and bundle this commit in with the one that introduces the initial broader set of promisified functions

jasnell · 2018-01-19T00:46:21Z

Ok, updated the implementation on this to hit all three of the points with one exception: if close fails during the garbage collection, a FatalException is used to crash the process because there is no JS stack to report that to. (At some point, it might make sense to try to capture where the failing FD is being opened if and only if it can be done efficiently).

This does not yet address @mcollina's last point about the fs.openFD() function. I'll deal with that once I'm sure the basic implementation of the FD class is sufficient.

ping @mcollina , @addaleax, @bnoordhuis

jasnell · 2018-01-19T00:57:55Z

src/node_file.cc

Note: if this stuff becomes more common, it may make sense to drop a couple of new utility functions in Environment... specifically... env->Resolve(promise, value) and env->Reject(promise, value)

(and various reject variants like, env->RejectUVException(promise, err, syscall, ...)

mcollina

I am LGTM on all but the fs.openFD(callback) API.
If it's callback-based, it should all be callback based. If it's promise-based, it should all be promise based.

I propose we keep this into fs.promise, and then we add an equivalent callback-based concept if it is needed.

@jasnell would it be better to open up a separate repo for this? So we can review the individual changes?

addaleax · 2018-01-19T11:11:32Z

doc/api/fs.md

I’d still go by my earlier point that this should ideally not be called FD … File might already be taken but I’d still kinda prefer something that would be more intuitive to people who haven’t done much UNIX/systems programming…

I'm open to suggestions :) I'm not good at the naming of things lol. Once had a cat named Cat.

@nodejs/collaborators Ideas? :)

FileDescriptor, FileHandle

Let's go with FileHandle

addaleax · 2018-01-19T11:12:46Z

src/node_file.cc

The last three lines are no-ops (assuming the CHECK(persistent().IsEmpty()) does not crash)

addaleax · 2018-01-19T11:15:29Z

src/node_file.cc

I’d prefer the uv_ namespace to remain reserved for libuv, otherwise it gets hard to tell which definitions are coming from where … also, it seems like you could just pass the message string as the data argument for the immediate

For the warning, sure, but not raising the UVException, which requires the errcode

Ah, right. Sorry!

addaleax · 2018-01-19T11:16:35Z

src/node_file.cc

This is not tearing down the process, it could still invoke uncaughtException handlers.

Hmm will have to test and verify that. I don't know if the uncaughtException handler will get triggered in this case

Ok yeah, uncaughtException does not catch the FatalException here. The process just dies.

Well, it prints the error at least, then dies :-)

$ ./node --expose-gc ~/tmp/test.js undefined:0 Error: EBADF: Closing file descriptor 12 on garbage collection failed, close

addaleax · 2018-01-19T11:18:11Z

src/node_file.h

This should inherit from ReqWrap<uv_fs_t>, like e.g. FSReqWrap does (or just inherit from the latter to begin with, that may or may not be easier)

Yeah, considered that but it makes it a bit more heavyweight than it really needs to be. Shouldn't be a problem tho.

addaleax · 2018-01-19T11:20:03Z

src/node_file.cc

How is async context propagated here? I can’t see anything? (same for resolve)

I know less than I need to on that part. What needs to be added here to propagate that?

@jasnell Once CloseReq is an ReqWrap, you’ll want a line like InternalCallbackScope callback_scope(this); before the resolve calls. That’s all. :)

(The idea is that the CloseReq is an AsyncWrap then, so it tracks state automatically, and you only need to enter a scope to make async_hooks listeners aware that things are happening in the CloseReq’s async context.)

addaleax · 2018-01-19T11:21:05Z

src/node_file.cc

Can you call this e.g. AfterOpenFD?

Yep, no problem

addaleax · 2018-01-19T11:22:25Z

src/node_file.cc

Can you use the non-deprecated overloads of Int32Value(), or just .As<Int32>()->Value()?

Yep. Will make that change in Open() also

jasnell · 2018-01-19T15:52:46Z

@mcollina ... understood on the JS api.. my plan is to get the basic implementation right for the C++ class here in this PR, but I'm going to move the actual commit into the FS promises PR along with everything else and not land this PR as is.

jasnell · 2018-01-19T16:06:36Z

Also, I don't think a separate repo is necessary. The changes are actually quite discreet are are no where near as massive as something like the http2 impl. One more PR should have the majority FS promise APIs implemented.

Currently, if any env->SetImmediate callback uses any of the env->Throw{Whatever} variants, the exception is lost and nothing happens. This commit catches those with a v8::TryCatch and does a FatalException to tear down the process.

In preparation for providinng a promisified fs API, introduce a FD wrapper class that automatically closes the file descriptor on garbage collection, helping to ensure that the fd will not leak. The object should still be explicitly closed, and a process warning will be emitted if it is closed on gc (a fatal exception will occur if closing during gc fails). The promisified fs API will use these objects instead of the numeric file descriptors in order to ensure that fd's are closed appropriately following promise resolve or reject operations.

jasnell · 2018-01-19T18:10:30Z

@addaleax ... updated :-)

jasnell · 2018-01-19T18:21:01Z

src/node_file.cc

+    }
+  } else {
+    // Already closed. Just reject the promise immediately
+    resolver->Reject(context, UVException(isolate, UV_EBADF, "close"));


@addaleax ... do we also need an InternalCallbackScope here? This is not using the CloseReq

jasnell · 2018-01-22T20:09:22Z

Closing in favor of #18297

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 11, 2018

jasnell commented Jan 11, 2018

View reviewed changes

vsemozhetbyt added the fs Issues and PRs related to the fs subsystem / file system. label Jan 11, 2018

Trott reviewed Jan 11, 2018

View reviewed changes

vsemozhetbyt reviewed Jan 11, 2018

View reviewed changes

jasnell force-pushed the fs-fd-object branch from 9e5d04e to 8e243e0 Compare January 15, 2018 17:10

mcollina reviewed Jan 17, 2018

View reviewed changes

jasnell force-pushed the fs-fd-object branch from e4cdd14 to 6904d76 Compare January 19, 2018 00:43

jasnell commented Jan 19, 2018

View reviewed changes

mcollina reviewed Jan 19, 2018

View reviewed changes

addaleax reviewed Jan 19, 2018

View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Jan 19, 2018

jasnell changed the title ~~fs: add FD object to manage file descriptors~~ [WIP] fs: add FD object to manage file descriptors Jan 19, 2018

jasnell added 3 commits January 19, 2018 10:09

[Squash] feedback

875d050

jasnell force-pushed the fs-fd-object branch from 6904d76 to 875d050 Compare January 19, 2018 18:10

jasnell commented Jan 19, 2018

View reviewed changes

jasnell closed this Jan 22, 2018

Uh oh!

[WIP] fs: add FD object to manage file descriptors #18109

[WIP] fs: add FD object to manage file descriptors #18109

Uh oh!

Conversation

jasnell commented Jan 11, 2018

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 11, 2018

Uh oh!

mcollina commented Jan 12, 2018

Uh oh!

addaleax commented Jan 12, 2018

Uh oh!

mcollina commented Jan 12, 2018

Uh oh!

addaleax commented Jan 12, 2018

Uh oh!

mcollina commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Jan 12, 2018

Uh oh!

mcollina commented Jan 12, 2018

Uh oh!

bnoordhuis commented Jan 12, 2018

Uh oh!

jasnell commented Jan 12, 2018

Uh oh!

addaleax commented Jan 12, 2018

Uh oh!

jasnell commented Jan 12, 2018

Uh oh!

bnoordhuis commented Jan 12, 2018

Uh oh!

mcollina commented Jan 12, 2018

Uh oh!

jasnell commented Jan 13, 2018

Uh oh!

jasnell commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 12, 2018 •

edited

Loading

jasnell Jan 19, 2018 •

edited

Loading