Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
brillout opened this issue Jul 7, 2023 · 20 comments
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@brillout
Copy link
Contributor

brillout commented Jul 7, 2023

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
Copy link
Contributor

climba03003 commented Jul 7, 2023

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
Copy link
Contributor Author

brillout commented Jul 7, 2023

Very neat.

the using keyword requires v8 support.

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

@silverwind
Copy link
Contributor

silverwind commented Jul 7, 2023

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
Copy link
Member

MoLow commented Jul 7, 2023

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
Copy link
Contributor

silverwind commented Jul 7, 2023

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@brillout
Copy link
Contributor Author

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 npm/rfcs#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".)

@aduh95 aduh95 changed the title Leverage using/Symbol.dispose to ensure resources are cleaned up before Node.js exists Leverage using/Symbol.dispose to ensure resources are cleaned up before Node.js exits Jul 12, 2023
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Jul 13, 2023
@cjihrig cjihrig removed the test_runner Issues and PRs related to the test runner subsystem. label Aug 11, 2023
Copy link
Contributor

github-actions bot commented Feb 8, 2024

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 github-actions bot added the stale label Feb 8, 2024
Copy link
Contributor

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 10, 2024
@brillout
Copy link
Contributor Author

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
Copy link

luchillo17 commented Apr 11, 2024

@benjamingr
Copy link
Member

@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
Copy link

@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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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 :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests

7 participants