Repository navigation
Plan of process.binding() deprecation #1482
Description
Activity
- I would advocate for
a, withbandcas possible intermediary steps. - You mean runtime-deprecated? It depends on the impact. If there is no popular module that uses any of them, we can deprecate all at once.
- If we plan to develop alternatives, we should not runtime-deprecate before the alternative is available. But once we are confident the alternative is enough, we can runtime-deprecate.
- Not sure non-node_module warnings would be useful in this case. Top-level apps probably don't use
process.binding.
- I would advocate for
I would advocate for a, with b and c as possible intermediary steps.
Note that there are multiple errors we can throw as the end-goal
- TypeError: process.binding is not a function
- TypeError: process.binding(...).foo is not a function
- Error [ERR_UNSUPPORTED_OPERATION]: process.binding() is not supported anymore
- [ERR_UNSUPPORTED_OPERATION]: process.binding(...).foo is not supported anymore
You mean runtime-deprecated? It depends on the impact.
Yes I guess that comes back to 3 (the ordering question)
Not sure non-node_module warnings would be useful in this case.
Libraries should still see the warning in their tests and get a reminder, if anyone is still watching for their test output (if they happen to run their tests with
--throw-deprecation, it's even more noticable).- I prefer (a) as the end result.
- I prefer (b) ... together at once
- Given my answers to 1 and 2, I prefer we land a runtime deprecation now (as per my PR)
- Just a regular runtime deprecation per my PR.
I couldn't find a way of doing incremental (synchronous) Zlib inflate on an open file stream using the built in node.js APIs so I reached for the Zlib binding.
My issue is that I have a stream with a bunch of deflate blocks. I know the offset of the start of the deflate block I need to inflate, but I don't know where the end of the deflate block is. While I could read in the whole file and pass that to
zlib.inflateSync, I'd much rather process it in chunks until I get to end end of the deflate stream.My implementation is something like (to others: don't use this... it's certainly not robust for general purpose applications):
function inflate( zlib: Zlib, // an `init`ialized instance of `process.binding("zlib").Zlib` state: Uint32Array, reader: Reader, // helper class to read from a file stream outSize: number, ) { const chunkSize = Math.max(constants.Z_MIN_CHUNK, outSize); const output = Buffer.allocUnsafe(outSize); let totalInflated = 0; let outOffset = 0; while (totalInflated < outSize) { const chunk = reader.peek(chunkSize); reader.seek(chunk.byteLength); let inOffset = 0; let availInBefore = chunk.byteLength; let availOutBefore = outSize - outOffset; // continue running while there is still data to process while (availInBefore > 0 && availOutBefore > 0) { // `Z_BLOCK` will process the zlib header and then return. The next time // it runs it will "inflate" up to the end of the input data, or the end // of the deflate block (which ever comes first) then return. It will // *not* continue to the next block of compressed data. const flush = constants.Z_BLOCK; zlib.writeSync( flush, chunk, inOffset, availInBefore, output, outOffset, availOutBefore, ); const [availOutAfter, availInAfter] = state; const inBytesRead = availInBefore - availInAfter; const outBytesWritten = availOutBefore - availOutAfter; inOffset += inBytesRead; outOffset += outBytesWritten; totalInflated += outBytesWritten; availInBefore = availInAfter; availOutBefore = availOutAfter; } } return output; }
Maybe there was a way and I just didn't see it?
Anyway, before officially deprecating this api a replacement would be really appreciated!
Reacted by Yein Sung@joyeecheung I wonder if V8 has an equivalent of
masqueradesAsUndefined? When you dotypeof openDatabasein Safari, it returnsundefinedand if you do!!openDatabaseit will returnfalse. But you can still call the function.Something like that might be useful as an in-between step for changes like this
@joyeecheung @jasnell given there was no progress on this front, can we get nodejs/node#50687 reopened and landed for v23?
I don't think I can commit enough time to this to get consensus and spin a vote, so I won't block if someone wants to reland that, but if someone else wants to initiate a vote I would vote -1 to console my conscience. For example this is what you see in a yarn + swc-node session if the Node.js release start to emit runtime warnings for
process.binding()unconditionally (the worst part IMO is the first one where the warning disturbs a prompt, and in the install session where the warning disturbs the progress bar)$ yarn init yarn init v1.22.22 question name (test-yarn): (node:5005) [DEP0111] DeprecationWarning: Access to process.binding('natives') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) question version (1.0.0): question description: question entry point (index.js): question repository url: question author: question license (MIT): question private: success Saved package.json Done in 4.43s. $ touch yarn.lock $ yarn set version berry (node:5026) [DEP0111] DeprecationWarning: Access to process.binding('natives') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5037) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) ➤ YN0000: You don't seem to have Corepack enabled; we'll have to rely on yarnPath instead ➤ YN0000: Downloading https://repo.yarnpkg.com/4.3.1/packages/yarnpkg-cli/bin/yarn.js ➤ YN0000: Saving the new release in .yarn/releases/yarn-4.3.1.cjs ➤ YN0000: Done with warnings in 0s 71ms $ yarn add @swc-node/register @swc/core typescript (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) ➤ YN0000: · Yarn 4.3.1 ➤ YN0000: ┌ Resolution step ➤ YN0085: │ + @swc-node/register@npm:1.10.3, @swc/core@npm:1.6.13, and 40 more. ➤ YN0000: └ Completed in 0s 297ms ➤ YN0000: ┌ Post-resolution validation ➤ YN0086: │ Some peer dependencies are incorrectly met by dependencies; run yarn explain peer-requirements for details. ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step ➤ YN0000: ⠼ =======================================----------------------------------------- (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. ➤ YN0000: ⠼ =============================================----------------------------------- (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. ➤ YN0000: ⠴ =========================================================----------------------- (node:5058) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. ➤ YN0013: │ 24 packages were added to the project (+ 103.57 MiB). ➤ YN0000: └ Completed in 1s 429ms ➤ YN0000: ┌ Link step ➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental ➤ YN0007: │ @swc/core@npm:1.6.13 [201f2] must be built because it never has been before or the last one failed ➤ YN0000: └ Completed in 0s 361ms ➤ YN0000: · Done with warnings in 2s 108ms $ echo 'console.log("Hello world")' > index.ts $ yarn node --loader @swc-node/register/esm index.ts (node:5115) [DEP0111] DeprecationWarning: Access to process.binding('util') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) (node:5126) [DEP0111] DeprecationWarning: Access to process.binding('fs') is deprecated. (Use `node --trace-deprecation ...` to show where the warning was created) Hello world
And I know that at least the fs binding use case (monkey-patching) is already known for years and no one has been actively working on it to my knowledge, so I don't see the plan of "let's deprecate and get requests" working out well for our users, because there is no guarantee how long it will take to actually release a solution to the request, meanwhile users have to suffer through disruptions like this in their CLIs, especially the ones that prompt and spin progress bars, and that looks very similar to the Buffer constructor situation we had a few years ago.
Reacted by David Murdoch and Yein Sung@joyeecheung That's why I changed the warning to
process.binding()instead of a warning to eachprocess.binding('key')call (nodejs/node#48568 (comment)). Wouldn't it solve it?The disruption to prompts and progress bars would be the same as long as the warning is emitted on anya
process.binding()call, as the CLI could invoke it while it is providing these interactive experiences. Notice how inyarn init, the package name prompt is disrupted by the warning aboutprocess.binding('natives'), and inyarn add, the progress bar is disrupted by the warning aboutprocess.binding('util'). And the warning won't go away any time soon because there is no guarantee that someone will shepherd through a fix to their request, given that the entire surface ofprocess.binding()is too big and there are cases like fs monkey patching that have never been addressed for years even though it's already known.This is what you'll see with nodejs/node#48568 in a colored console (I am not sure why I am doing this, research like this should be part of any PR proposing warnings to
process.bindingIMO)
@joyeecheung apart from yarn, where you able to catch what other popular module was affected?
I am pretty sure whoever proposing the breakage can easily try it out on other popular tools and report back. I don’t want to do it myself because
- I am just doing it for my good conscience for trying not to ruin the UX for our users. My conscience is worth an easy investigation of a use case I know, but not really worth a significant bigger amount of time surveying the ecosystem. IMO it should be done by who’s proposing the breakage.
- I think something would be wrong with the process if there is no obligation for whoever proposing a breakage known to be potentially high-impact to do any impact research or developing alternative APIs at all, and if no one else does the research or develops the alternative API, instead of finishing the work we just forget it and proceed breaking users. That already happened with Buffer constructor before. What’s worse with this case is that unlike the Buffer constructor which had alternative APIs, with process.binding() there are known popular use case(like fs monkey patching) with no alternative API to migrate to, no one is working on developing an alternative API, and we are intentionally leaving users living with the warning for an indefinite amount of time. Also, the Buffer constructor change was justified by security reasons, but this is rushed due to unwillingness to do impact research or develop migration paths, it’s not under any emergency AFAIK.
If others think that the breakage is acceptable without further investigation from whoever proposing the change, and we should just leave users with the warning without any commitment into developing a migration path, I rest my case. I can reconcile with my conscience with the work I committed so far.
Reacted by Matteo Collina and Yein SungThanks @joyeecheung, I concur.
I know this is still an unresolved issue, should we keep this open? Should we transition it to a strategic initiative? There's been no activity in this issue for a year now so keeping it open indefinitely likely has no value. Just think we need to identify some concrete next steps.
End user here: if the API is going to be deprecated a replacement is needed first. If that isn't in the cards, the concrete next steps should be to close the issue as "not planned".
@davidmurdoch ... We have, over the course of several years, been incrementally replacing various apis that were available via
process.binding(...)with public APIs where appropriate. Theprocess.binding(...)API itself should not be replaced with a public API tho ... that's actually the point of deprecating it. We eventually want to completely remove the ability to get internal native bindings from visible public API at all. If there are still cases where the ecosystem needs access toprocess.binding(...)for any particular use case, it's most helpful to have issues opened so that we're aware of those and can investigate.Ok, I've only ever needed the zlib bindings, since the public API doesn't provide
writeSyncI had previously commented on it here: #1482 (comment) , though I haven't followed up to check if the method has since been exposed in the latest versions of Node (or if there are now any other alternatives available).Reacted by James M SnellGiven the findings of the breakage I think the plan proposed in 2021 and mentioned in the OP is still valid:
- Do comprehensive research into what exact features ecosystem modules get through process.binding() (this isn't actually going to be a huge API surface!)
- Patch process.binding() so that it returns objects that cover those use cases, implemented in terms of public APIs
- Optionally, runtime-deprecate process.binding() only for the cases that users aren't commonly encountering
- Eventually, remove support for those cases specifically
- Leave it at that, don't runtime-deprecate process.binding() as a whole and leave the shim in place forever
It would be nice if someone wants to champion this as a strategic initiative. But if no volunteer shows up I am okay with just closing it - this issue was just opened under suggestion of nodejs/node#50687 (comment). Overall I would be +1 to whoever wanting to carry out the 2021 plan, and still -1 to a PR that just emits runtime warnings and potentially breaks a lot of CLI tools without any impact research or way-paving. There is some maintenance burden of
process.binding()but straight up breaking CLI tools in a major release, having to deal with all the user complaints, and only starting to come up with alternative APIs a posteriori sounds like an even bigger maintenance burden and churn - I suspect that might also lead to rushed alternatives compared to ones that could be given proper thoughts to before the breakage happens.Reacted by James M Snell and David MurdochReacted by David Murdoch
This issue is not new but motivated by nodejs/node#50687, which proposed to
For background in 2021 the plan suggested in nodejs/node#37485 (review) and followed since nodejs/node#37819 was:
I am opening this issue trying to slice the issue into different questions instead of bundling all together into just two different plans, in the hope of figuring out a better compromise.
There are several questions to the deprecation of
process.binding():a) Delete
process.bindingcompletely so that when trying to invoke it, users get an error.b) A function that does no more than emitting a warning and returning empty objects. Users almost always get an error when they try to access an unsupported property, but if the code path is unused, e.g. only loading a
process.binding()from the top-level but never actually hit the code path using anything from it, it would not fail.c) Leave a low-maintenance and harmless shim in place forever. Users get an error only when they try to use a specific unsupported property.
a) Individually, based on how problematic they are/how difficult it is to provide alternatives
b) Together at once
a) We investigate the impact (e.g. through CITGM), develop and release alternatives on the way, monitor the changes in the impact, before deciding when it's ready to runtime-deprecate it (individually or all together).
b) Runtime-deprecate it directly and mitigate the impact afterwards based on the bug reports/feature requests we receive.
a) Emit runtime warnings unconditionally
c) Non-node_module warnings first, node_module warnings in the next cycle
I think we as @nodejs/tsc need to take a deeper look into these questions and make some decisions before finishing a plan of the deprecation.