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: upgrade process.binding to runtime deprecation #37485

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 22, 2021

@nodejs/tsc ... this one still may be controversial but I figured I'd give it a shot.

process.binding() has always been problematic and has been deprecated since the 11.x timeframe. We have fully migrated to internalBinding() internally but there are still a few stragglers in the ecosystem that have not let go of their hold on our internal bindings. It's time to start nudging them harder out of the nest.

Fixes: #30884

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 22, 2021
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.

In undici we are using it to access the internal http parser. It would be good to get some way to retrieve it.

cc @ronag

@ronag
Copy link
Member

ronag commented Feb 23, 2021

Yep. Removing would break undici.

@devsnek
Copy link
Member

devsnek commented Feb 23, 2021

can't you just depend on llhttp? it's kept in a separate repo and everything.

@jasnell
Copy link
Member Author

jasnell commented Feb 23, 2021

The approach for undici then should be to open a PR that exposes a public API for interacting with the http parser. Using process.binding() directly is not supported and should not be used.

(or what @devsnek said)

@ChALkeR
Copy link
Member

ChALkeR commented Feb 23, 2021

While +1 in general, process.binding('constants') still seems used in popular packages, let's patch those. Also, it seems safe.

Perhaps we could move forward by runtime-deprecating everything except a list of some exceptions (for undici / constants / other usecases that we would find)? That would still improve the situation and perhaps should be much easier to get consensus on?

That is, if we don't resolve those by the time of the next major release.

Upd: will recheck constants in a bit, that could already be fixed (that was used in older Emscripten-generated code, could be fixed now there, or not).

@ronag
Copy link
Member

ronag commented Feb 23, 2021

can't you just depend on llhttp? it's kept in a separate repo and everything.

We don’t want native deps. What we would like is to have a web assembly build of llhttp. But we need help for that...

@ronag
Copy link
Member

ronag commented Feb 23, 2021

Also there is the consume/unconsume optimization which I’m not sure is possible without process.binding.

In that case we would have to bring undici into core or somehow expose that functionality.

@targos
Copy link
Member

targos commented Feb 23, 2021

we would have to bring undici into core

This would be a reasonable move, IMO

@ronag
Copy link
Member

ronag commented Feb 23, 2021

we would have to bring undici into core

This would be a reasonable move, IMO

The biggest task with that is to port all the tests from tap.

I’m actually sceptical to whether bringing undici into core give any value other than:

  1. Get rid of process.binding hack
  2. Maybe encourage more usage.
  3. Maybe get more collaborators involved.

Doing it just for 1 seems a bit inefficient in terms of time and effort.

@targos
Copy link
Member

targos commented Feb 23, 2021

I would add:

  1. A better, more modern and faster builtin API for HTTP requests.

@ronag
Copy link
Member

ronag commented Feb 23, 2021

So could this be a way forward?

  1. Runtime deprecate process.binding but add an exception for undici?
  2. Look into moving undici into core.

I'm happy with having undici both inside and outside of core. My main concern is the tests which I don't quite know what to do about. Would it be acceptable to have tap based tests in core?

@targos
Copy link
Member

targos commented Feb 23, 2021

I would not be against adding tap in the repo for tests. Happy to look into it. We are talking about https://github.com/tapjs/node-tap, right?

@aduh95 aduh95 modified the milestone: , Feb 23, 2021
@jasnell
Copy link
Member Author

jasnell commented Feb 23, 2021

I'd rather not carve out any special cases as it just makes things more difficult. The viable alternatives that I can see are either: a) bring undici into core, b) expose the http parser as a supported core module, c) have undici depend on llhttp directly. Given that I believe there are several libraries that are getting to the http parser the same way, option (b) may actually be the better way to go.

@devsnek
Copy link
Member

devsnek commented Feb 23, 2021

@ronag i think by default the build output is just plain js, but i could be wrong cc @indutny

@ronag
Copy link
Member

ronag commented Feb 23, 2021

@ronag i think by default the build output is just plain js, but i could be wrong cc @indutny

I think we lose performance that way.

@jasnell
Copy link
Member Author

jasnell commented Feb 23, 2021

Oh, just remembered... it is also possible to get to the internal HTTPParser via require('_http_common')

root@DESKTOP-5KK9VIR:~/node/node# ./node -pe "require('_http_common').HTTPParser"
[Function: HTTPParser] {
  REQUEST: 1,
  RESPONSE: 2,
  kOnMessageBegin: 0,
  kOnHeaders: 1,
  kOnHeadersComplete: 2,
  kOnBody: 3,
  kOnMessageComplete: 4,
  kOnExecute: 5,
  kOnTimeout: 6
}

That would likely be the most immediate way of avoiding the direct use of process.binding(). It's still not a supported public API so may still run into issues down the road, but it addresses the most immediate concern

@devsnek
Copy link
Member

devsnek commented Feb 23, 2021

wasm seems to work at least 😄

@ronag
Copy link
Member

ronag commented Feb 23, 2021

How do I do that?

@devsnek
Copy link
Member

devsnek commented Feb 23, 2021

@ronag
Copy link
Member

ronag commented Feb 23, 2021

Could we get the wasm build on npm?

@devsnek
Copy link
Member

devsnek commented Feb 23, 2021

I'm not planning to do so but I think as an org yes that is possible. We already publish readable-stream, for example.

@ronag
Copy link
Member

ronag commented Feb 23, 2021

llhttp is already on npm. Though a bit outdated and lacking wasm.

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

I tested that we can use _http_common in Undici. Long term we will look into the wasm-based solution.

@targos
Copy link
Member

targos commented Feb 24, 2021

This is probably going to annoy many people but maybe that's necessary so the ecosystem migrates out of it.
For example, node-core-utils currently prints a pending deprecation warning because of https://www.npmjs.com/package/external-editor, which depends on an old version of https://www.npmjs.com/package/tmp which uses process.binding.

@jasnell
Copy link
Member Author

jasnell commented Feb 24, 2021

And yet tmp was patched to avoid that back in 2018 (raszi/node-tmp@f2609c9)

@Trott
Copy link
Member

Trott commented Feb 24, 2021

And yet tmp was patched to avoid that back in 2018 (raszi/node-tmp@f2609c9)

This is a common theme, as you probably are aware. In the current download stats (screenshot below), you can see that the current tmp@0.2.1 has an impressive 4 million downloads in the last 7 days...but it's a fraction of the nearly 13 million downloads achieved in the last 7 days by 0.0.33 published 4 years ago.

image

@jasnell jasnell added the review wanted PRs that need reviews. label Feb 25, 2021
@jasnell
Copy link
Member Author

jasnell commented Feb 25, 2021

Yep... thus the reason why I said "It's time to start nudging them harder out of the nest." in the original post ;-)

@jasnell
Copy link
Member Author

jasnell commented Feb 25, 2021

@nodejs/tsc ... this needs additional review please.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 26, 2021

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I wish we could do this, but this isn't going to work without a plan for handling situations like the tmp one.

The only reasonable way forward here that I see, is:

  1. Do comprehensive research into what exact features ecosystem modules get through process.binding() (this isn't actually going to be a huge API surface!)
  2. Patch process.binding() so that it returns objects that cover those use cases, implemented in terms of public APIs
  3. Optionally, runtime-deprecate process.binding() only for the cases that users aren't commonly encountering
  4. Eventually, remove support for those cases specifically
  5. Leave it at that, don't runtime-deprecate process.binding() as a whole and leave the shim in place forever

@Flarna
Copy link
Member

Flarna commented Mar 2, 2021

I found following uses in my code:

  • process.binding("natives"): I use only the keys. Most likely I can replace it by module.builtinModules but the difference is that it doesn't include internal modules
  • process.binding("pipe_wrap").Pipe: I use socket._handle instanceof Pipe to check if a socket is connected to a Pipe. Can't remember why I did it that way but I assume I will find a different approach.

@jasnell
Copy link
Member Author

jasnell commented Mar 8, 2021

Closing this in favor of a more incremental approach

@jasnell jasnell closed this Mar 8, 2021
addaleax added a commit to addaleax/node that referenced this pull request Mar 19, 2021
addaleax added a commit that referenced this pull request Mar 27, 2021
Ref: #37485 (review)
Ref: #37787

PR-URL: #37819
Refs: #37787
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review wanted PRs that need reviews. 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.

move things away from process.binding('util')