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

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

Merged
merged 20 commits into from
Jul 11, 2024

Conversation

H4ad
Copy link
Member

@H4ad H4ad commented May 31, 2024

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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot 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
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved
@H4ad H4ad added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 1, 2024
@H4ad H4ad marked this pull request as ready for review June 1, 2024 15:20
@H4ad H4ad requested a review from mcollina June 1, 2024 18:05
LICENSE Show resolved Hide resolved
doc/api/process.md Show resolved Hide resolved
lib/internal/process/finalization.js Show resolved Hide resolved
lib/internal/process/finalization.js Outdated Show resolved Hide resolved
lib/internal/process/finalization.js Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024
@nodejs-github-bot
Copy link
Collaborator

@atlowChemi atlowChemi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@H4ad
Copy link
Member Author

H4ad commented Jun 9, 2024

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

@H4ad H4ad added the blocked PRs that are blocked by other issues or PRs. label Jun 9, 2024
@H4ad
Copy link
Member Author

H4ad commented Jun 9, 2024

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

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
beforeExit: onBeforeExit,
};

function install(event) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should emit an experimental warning here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added on register, registerBeforeExit and unregister.

@mcollina
Copy link
Member

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)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@H4ad
Copy link
Member Author

H4ad commented Jul 11, 2024

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.

@H4ad H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@H4ad 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 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 nodejs-github-bot merged commit 05bb4a7 into nodejs:main Jul 11, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 05bb4a7

@H4ad H4ad deleted the add-on-exit-leak branch July 11, 2024 20:45
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
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>
aduh95 added a commit that referenced this pull request Jul 12, 2024
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: TODO
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
aduh95 added a commit that referenced this pull request Jul 12, 2024
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
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
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>
aduh95 added a commit that referenced this pull request Jul 16, 2024
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
aduh95 added a commit that referenced this pull request Jul 16, 2024
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
aduh95 added a commit that referenced this pull request Jul 16, 2024
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
aduh95 added a commit that referenced this pull request Jul 17, 2024
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
RafaelGSS pushed a commit that referenced this pull request Jul 17, 2024
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
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
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>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
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
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.