nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.5k stars 29.56k forks source link

Leverage `using`/`Symbol.dispose` to ensure resources are cleaned up before Node.js exits #48687

Closed brillout closed 7 months ago

brillout commented 1 year ago

What is the problem this feature will solve?

Today, there isn't a way to guarantee resource cleanup. For example:

export function doSomeWork() {
    const path = ".some_temp_file";
    const file = fs.openSync(path, "w+");

    try {
        // use file...

        if (someCondition()) {
            // do some more work...
            return;
        }
    }
    finally {
        // Close the file and delete it.
        fs.closeSync(file);
        fs.unlinkSync(path);
    }
}

If the process exits between fs.openSync() and the finally code block, then the file isn't removed.

What is the feature you are proposing to solve the problem?

While a 100% guarantee isn't possible, leveraging the new using keyword, there is an opportunity to dramatically increase the probability of successful resource cleanup.

Is that on Node.js's radar?

What alternatives have you considered?

No response

climba03003 commented 1 year ago

Is that on Node.js's radar?

You can see multiple pull request already working on it and most of them are merged.

The first iteration of Symbol.dispose is released on 20.4.0, but the using keyword requires v8 support. https://bugs.chromium.org/p/v8/issues/detail?id=13559 https://bugs.chromium.org/p/v8/issues/detail?id=13879

brillout commented 1 year ago

Very neat.

the using keyword requires v8 support.

Makes sense. A workaround is to use TypeScript 5.2 which supports using.

silverwind commented 1 year ago

Would such a cleanup work even when v8 crashes like in heap out of memory cases, e.g. when no JS execution is possible any more.

MoLow commented 1 year ago

Would such a cleanup work even when v8 crashes like in heap out of memory cases, e.g. when no JS execution is possible any more.

Probably not

silverwind commented 1 year ago

Would such a cleanup work even when v8 crashes like in heap out of memory cases, e.g. when no JS execution is possible any more.

Probably not

I guess it would if the cleanup is performed in C++. That would be a truly reliable way for cleanup as the process exit hook is not triggered on V8 crash, but I guess another way to declare the resources will be needed as JS can not execute anymore then unless v8 is restarted which I would not advice to do.

benjamingr commented 1 year ago

Hey,

Great to see users are asking for this!

As others have mentioned this is "on the roadmap" and we started adding Symbol.dispose and Symbol.asyncDispose support to all APIs. You can using timers, readable streams, file handles and mock timers so far.

We've done this in collaboration with the TypeScript team and coordination with Babel so users in transpilers would be able to benefit from it before V8 lands the syntactic support.

When the process crashes, electricity goes down or another catastrophic failure happens you cannot rely on disposers running. It is effectively a safer/neater alternative for try... finally.

silverwind commented 1 year ago

When the process crashes, electricity goes down or another catastrophic failure happens you cannot rely on disposers running. It is effectively a safer/neater alternative for try... finally.

Is there a difference between v8 crashing and node crashing? Couldn't node still run cleanup before exiting in case of v8 crash?

benjamingr commented 1 year ago

I'm wondering if we should ship a defer helper since you can absolutely await using the file in your case which would close it but you would have to wrap it or try/finally it in order to also unlink it.

benjamingr commented 1 year ago

Is there a difference between v8 crashing and node crashing? Couldn't node still run cleanup before exiting in case of v8 crash?

My point is that users cannot rely on app-level cleanup code running in all cases since we (and any other platform) cannot guarantee that it will run. There are some cases where cleanup will run (e.g. unhandled rejection since it would propagate through the stack and disposers would run) and some cases where they won't (e.g. segfault).

benjamingr commented 1 year ago

What APIs would you expect support in other than streams/files @brillout ?

brillout commented 1 year ago

Thanks for the ping. My only use case so far is clear up of temporary files, but I'm sure there are other use cases that I can't think of right now.

(Unrelated but since you're Node.js member, I wonder whether the Node.js team can influence the outcome of https://github.com/npm/rfcs/issues/665? It's a really bad situation. It's causing massive pain on a daily basis to Node.js users. If we can find a solution that would significantly address the reputation of "JavaScript is a mess".)

github-actions[bot] commented 8 months ago

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] commented 7 months ago

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

brillout commented 7 months ago

Are there any docs on this? Googling Symbol.dispose site:https://nodejs.org or using keyword site:https://nodejs.org doesn't lead to any relevant docs.

Does the latest Node.js version support (somehwat) guaranteed temporary file removal?

luchillo17 commented 6 months ago

These are the only references I could find, not much to go off, its basically so new that there are no docs about it: https://nodejs.org/api/timers.html#immediatesymboldispose https://nodejs.org/api/events.html#eventsaddabortlistenersignal-listener https://nodejs.org/api/child_process.html#subprocesssymboldispose https://github.com/search?q=repo%3Anodejs%2Fnode+symbol.dispose&type=code

benjamingr commented 6 months ago

@luchillo17 note that for some cases we expose Symbol.asyncDispose where appropriate https://nodejs.org/api/stream.html#readablesymbolasyncdispose

The main issue in terms of DX is that because we're a JS and not a TS runtime our docs are for JS and thus we don't have good examples until this lands in JS land.

luchillo17 commented 6 months ago

@benjamingr Worry not, I'm not gunning for this feature just yet, I know tc39 is in stage 3 draft, I just came here out of curiosity as someone was showing it in the context of a TS unit test for teardown logic.

brillout commented 6 months ago

Is there anything available for deleting temporary files? This would quite nice for both Vite and Vike. (Temporary files not being cleaned up is a common issue.)

benjamingr commented 6 months ago

@brillout I maintain a package (tmp-promise) with a disposer pattern that could use an update but I'm not aware of anything in core.

I think we can probably ship a promises disposable version of mkdtemp if that's common enough

(also TIL about vike, happy 10000 to me I guess :))

brillout commented 6 months ago

Neat. Yea, I guess it needs to be a core thing, so that Node.js knows it should apply the cleanup if, for example, the user terminates the process by hitting ctrl-c before the disposable resolves.

(Thank you, I'm glad Vike resonates with you :))