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

lib: promote process.binding/_tickCallback to runtime deprecation #50687

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 12, 2023

These have been pendingDeprecation for long enough.

This also adds the --deprecated-process-binding (and matching --no-deprecated-process-binding) CLI option as a future transition path for when we do ultimately remove process.binding().

--deprecated-process-binding ensures that process.binding(...) is available and is currently the default. In the future, when we are ready to begin moving process.binding(...) to EOF, we can flip the default so that process.binding('...') is unavailable by default but still able to be switched back on if necessary, giving users an escape hatch if they really need it. This will give us a more nuanced path than abruptly removing it completely.

The current transition path would be:

  • Ship the runtime deprecation in Node.js v22 --deprecated-process-binding on by default.
  • --no-deprecated-process-binding becomes the default in Node.js v23
  • process.binding(...) is removed entirely in Node.js v24, with the CLI option becoming a no-op

/cc @nodejs/tsc

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/startup

@nodejs-github-bot 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. needs-ci PRs that need a full CI run. labels Nov 12, 2023
@nodejs-github-bot
Copy link
Collaborator

doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the runtime-deprecate-process-binding-next branch from dc149cd to 957fc0f Compare November 12, 2023 15:21
These have been pendingDeprecation for long enough.

This also adds the `--deprecated-process-binding` (and matching
`--no-deprecated-process-binding`) CLI option as a future transition
path for when we do ultimately remove `process.binding()`.

`--deprecated-process-binding` ensures that `process.binding(...)`
is available and is currently the default. In the future, when we
are ready to begin moving `process.binding(...)` to EOF, we can
flip the default so that `process.binding('...')` is unavailable
by default *but still able to be switched back on if necessary*,
giving users an escape hatch if they really need it. This will give
us a more nuanced path than abruptly removing it completely.
@jasnell jasnell force-pushed the runtime-deprecate-process-binding-next branch from 957fc0f to aed9d49 Compare November 12, 2023 15:22
@joyeecheung
Copy link
Member

joyeecheung commented Nov 12, 2023

I think we should have alternative suggestions before we runtime-deprecate these. Otherwise I very much suspect that these are used commonly enough that no migration would be done (because there aren't alternatives) and many users would just disable the warnings....

@jasnell
Copy link
Member Author

jasnell commented Nov 12, 2023

What kind of alternative suggestions would you have in mind? I'm afraid that unless we just go ahead and proceed with runtime deprecating these there's never going to be enough pressure for the ecosystem to actually move off of them. As it is these are already quite dangerous to use.

@jasnell jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 12, 2023
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @jasnell.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 12, 2023

As it is these are already quite dangerous to use.

"These" can be a bit too generic - process.binding() contains 28 bindings, many include read-only constants that probably can't do much damage. At least I think they deserve individual deprecations, or we can classify them at least based on side effects. We already have something in place like lib/internal/legacy/processbinding.js which I think is a nicer plan compared to just deprecate all bindings at once without actually looking into them (or I don’t see strong motivation to abandon this plan).

@jasnell
Copy link
Member Author

jasnell commented Nov 12, 2023

I remember many years ago that was the intended plan but there never seemed to be any follow up to complete that kind of effort, and it seems obvious that having it behind pendingDeprecation hasn't helped (do we actually know if anyone actually uses pendingDeprecation?). At the very least, marking these as runtime deprecations should make it far easier to find and convert those various uses to properly supported APIs where necessary, and the inclusion of the command-line flag (together with --no-deprecation) should address the cases where users aren't ready to drop these. I guess I'm just no longer convinced that a more incremental approach really advances the ball forward enough

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

@@ -144,6 +143,9 @@ const experimentalModuleList = new SafeSet();
{
const bindingObj = { __proto__: null };

// TODO(@jasnell): If the --deprecated-process-binding option is false we really
Copy link
Member

Choose a reason for hiding this comment

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

You can just expose the processBindingAllowList list, which is set to empty initially, and add things back in pre-execution. We do something similar for --expose-internals for example.

// Ideally it wouldn't be defined at all already but realm.js does not
// provide a means of checking to see if the option is enabled or not,
// so we do it here.
delete process.binding;
Copy link
Member

Choose a reason for hiding this comment

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

This should at least throw a warning and tell users about the option --deprecated-process-binding.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

I think we haven't done the due diligence for this change. If the reason for abandoning the incremental plan is that it's not moving, then I suggest we:

  1. Open a tracking issue for auditing processBindingAllowList and moving them to legacyWrapperList with wrappers added in lib/internal/legacy/processbinding.js
  2. Ask for help in the audit - this look like a list of good first contributions to me. The rule can be simple - existing read-only constants and side-effect-free helpers can be exposed as-is before we think about alternatives. Any method beyond that gets filtered out. If somehow this causes significant regression, we can get the filtered stuff back out quickly and add a TODO for proper alternative - which can just be semver-patch. This also helps to make sure that no new additions to the bindings are going to end up accessible to the user land going forward.
  3. We set a deadline for the audit. This won't be released until next April anyway. I think we should defer the deadline until at least next February. When the deadline comes, for any of the bindings still in processBindingAllowList, process.binding returns an empty object and emits a warning if --deprecated-process-binding isn't set, or return the original one if the flag is set. Eventually, process.binding() should just return a list of harmless wrappers. We can revisit whether we should delete this list of harmless wrappers in the future.

If we get some help in the auditing, 4 months look enough for auditing to me.

@RafaelGSS
Copy link
Member

Related work #48568

@mcollina
Copy link
Member

@joyeecheung I think it's a solid plan if we can get volunteers for the audit. Can you open a "call to action" issue describing what needs to be done?

@joyeecheung
Copy link
Member

Can you open a "call to action" issue describing what needs to be done?

I think I'll need consensus that this is the path instead before I open such issue to avoid making contributors confused/waste audit work. Should I call for a vote or something?

@jasnell
Copy link
Member Author

jasnell commented Nov 24, 2023

Should I call for a vote or something?

We may need one, yes. Not sure how else we'll come to consensus here.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 6, 2023

Discussed again in the TSC meeting, the idea agreed on is to open an issue in the TSC repo regarding the plan of process.binding() (note that this is about process.binding(), not process._tickCallback). As least discuss about the various steps that could be taken towards the eventual deprecation, e.g.

  1. whether we need legacy wrappers (which was the original plan from process: upgrade process.binding to runtime deprecation #37485 (review))
  2. how the warnings can be rolled out (application-only or dependency too)
  3. how the flags should be managed
  4. whether assessment of ecosystem impact should be done prior to runtime deprecation
  5. whether the 20 or so bindings should he deprecated individually or all together
  6. whether read-only constants should be left around
  7. whether we should provide proper alternatives before moving on with runtime deprecation

I'll try to come up with a an issue that summarizes the options and get a vote together (maybe on the steps first before we combine the steps together to form some plan to vote on)

@joyeecheung
Copy link
Member

Moved to nodejs/TSC#1482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants