process: port on-exit-leak-free to core by H4ad · Pull Request #53239 · nodejs/node · GitHub

This PR basically ports on-exit-leak-free to core, in their words:

This module helps dispose of an object gracefully when the Node.js process exits. It executes a function with a given parameter on 'exit' without leaking memory, cleaning things up appropriately if the object is garbage collected.

This module was created by @mcollina, so all the credits for him, I'm just doing the port.

Motivation

This library was proposed to be introduced on core previously on issue #48058 via the following syntax:

new FinalizationRegistry(fn, {
  runOnExit: true
})

But the main arguments against introducing it were:

By @tniessen: The usage of FinalizationRegistry is an anti-pattern, not recommended to close files automatically, and best to avoid when possible (said by the authors of the API) (ref comment)
By @aduh95: Needing a strong motivation, do not change the FinalizationRegistry since it is a standard. (ref comment)

To address the arguments above, I made some changes:

Instead of changing the standard, I introduced finalization object inside process object that wraps the methods of on-exit-leak-free.
The motivation to push this module started at on-exit-leak-free, copy or move to core? H4ad/nodejs-logging-proposal#10, where I'm trying to create a proposal to introduce a logging module to core, and this dependency is one of the dependencies that we plan to use in the implementation. Also, the module on-exit-leak-free has more than 5M downloads per month, so it's a demanded feature.

APIs Introduced

Now we have three new methods of process.finalization:

register: Accepts two arguments, objand fn, and executes the fn when the exit event is emitted.
- The fn is a function that will be called with obj registered and the event, which is exit.
registerBeforeExit: Accepts two arguments, objand fn, and executes the fn when the beforeExit event is emitted.
- The fn is a function that will be called with obj registered and the event, which is beforeExit.
unregister: Accepts one argument obj to unregister the object.

About moving away from classes (like aduh said about creating a new one), I think exposing functions was better than creating a class since we want to be able to use/call those methods in any place we want. But I'm open to better API design for these features.

TODO

I will wait for a few folks to manifest about introducing this feature (in the current state) before spending time creating documentation that could be totally changed or not used, so I will keep it as a draft for a couple of days.

Update the documentation
- Explain how to use
- Link/Include the comments about "avoid where possible" that were said by the authors of the FinalizationRegistry/WeakRef.
- Include examples of how to use this feature.

Review requested:

@nodejs/startup

Given this is per-thread, you'd need to add some test that verifies this for multi-threading scenarios.

Can you add a reference that those were extracted by my module, with its license? Thx.

lgtm

CI: https://ci.nodejs.org/job/node-test-pull-request/59706/

CI: https://ci.nodejs.org/job/node-test-pull-request/59707/

CI: https://ci.nodejs.org/job/node-test-pull-request/59708/

/cc @nodejs/tsc pinging to give some visibility on this change since it introduces something new to process.

Added block until friday (06/14) to see if there's any objection, otherwise I will remove blocked and merge.

LGTM!

Left a couple of comments, but, it's still unclear to me why we need it on core instead of using the npm package directly. It's not very common usage by Node.js developers I assume.

FWIW I will review it properly before the TSC meeting.

I think we should emit an experimental warning here.

Added on register, registerBeforeExit and unregister.

Left a couple of comments, but, it's still unclear to me why we need it on core instead of using the npm package directly. It's not very common usage by Node.js developers I assume.

The end goal of @H4ad is to add a logging utility to Node.js. This is needed to do it properly, i.e. flush all the buffered logs when 'exit' is emitted. (logs will be missed if this is not done)

lgtm

Force pushed due to the error on mac-os test.

@cjinhuo I posted a comment on your issue, but try reading the docs that this PR introduces about this feature, maybe it can help you understand what is going on.

CI: https://ci.nodejs.org/job/node-test-pull-request/60247/

Landed in 05bb4a7

PR-URL: #53239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 module: * add __esModule to require()'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #53826

PR-URL: #53239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826

PR-URL: nodejs#53239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) nodejs#53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) nodejs#53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) nodejs#52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) nodejs#52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) nodejs#53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) nodejs#53462 test_runner: * support glob matching coverage files (Aviv Keller) nodejs#53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) nodejs#53682 PR-URL: nodejs#53826

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels May 31, 2024

mcollina reviewed Jun 1, 2024

View reviewed changes

Comment thread lib/internal/process/per_thread.js Outdated

H4ad added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 1, 2024

H4ad marked this pull request as ready for review June 1, 2024 15:20

H4ad requested a review from mcollina June 1, 2024 18:05

anonrig reviewed Jun 1, 2024

View reviewed changes

Comment thread LICENSE Outdated

mcollina requested changes Jun 2, 2024

View reviewed changes

Comment thread doc/api/process.md Outdated

atlowChemi reviewed Jun 2, 2024

View reviewed changes

Comment thread lib/internal/process/finalization.js Outdated

atlowChemi reviewed Jun 2, 2024

View reviewed changes

Comment thread lib/internal/process/finalization.js Outdated

Comment thread lib/internal/process/finalization.js Outdated

Comment thread lib/internal/process/finalization.js Outdated

mcollina requested changes Jun 4, 2024

View reviewed changes

Comment thread doc/api/process.md Outdated

mcollina approved these changes Jun 8, 2024

View reviewed changes

atlowChemi approved these changes Jun 9, 2024

View reviewed changes

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024

atlowChemi added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 9, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024

H4ad added the blocked PRs that are blocked by other issues or PRs. label Jun 9, 2024

ShogunPanda approved these changes Jun 10, 2024

View reviewed changes

RafaelGSS reviewed Jun 10, 2024

View reviewed changes

mcollina approved these changes Jun 11, 2024

View reviewed changes

H4ad added 2 commits July 11, 2024 08:24

attempt to improve the docs

c094345

remove stdout usages

8002996

H4ad force-pushed the add-on-exit-leak branch from a9b9fe6 to 8002996 Compare July 11, 2024 11:42

H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024

anonrig approved these changes Jul 11, 2024

View reviewed changes

H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 11, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2024

nodejs-github-bot merged commit 05bb4a7 into nodejs:main Jul 11, 2024

H4ad deleted the add-on-exit-leak branch July 11, 2024 20:45

aduh95 mentioned this pull request Jul 12, 2024

V22.5.0 proposal #53826

Merged

Araxeus mentioned this pull request Jul 18, 2024

Not cleaning things up... mcollina/on-exit-leak-free#31

Open

targos added the dont-land-on-v20.x label Sep 21, 2024

H4ad mentioned this pull request Jan 18, 2025

Should we deprecate this package? mcollina/on-exit-leak-free#65

Open

mcollina mentioned this pull request Oct 4, 2025

lib: create util.log function #60116

Closed

process: port on-exit-leak-free to core by H4ad · Pull Request #53239 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: port on-exit-leak-free to core - #53239

process: port on-exit-leak-free to core#53239nodejs-github-bot merged 20 commits intonodejs:mainnodejs/node:mainfrom H4ad:add-on-exit-leakH4ad/node:add-on-exit-leakCopy head branch name to clipboard

Conversation

H4ad commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

APIs Introduced

TODO

Uh oh!

nodejs-github-bot commented May 31, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jun 9, 2024

Uh oh!

nodejs-github-bot commented Jun 9, 2024

Uh oh!

nodejs-github-bot commented Jun 9, 2024

Uh oh!

H4ad commented Jun 9, 2024

Uh oh!

H4ad commented Jun 9, 2024

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RafaelGSS Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

H4ad Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 10, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

H4ad commented Jul 11, 2024

Uh oh!

nodejs-github-bot commented Jul 11, 2024

Uh oh!

nodejs-github-bot commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

process: port on-exit-leak-free to core#53239
nodejs-github-bot merged 20 commits into
nodejs:mainfrom
H4ad:add-on-exit-leak

H4ad commented May 31, 2024 •
edited
Loading