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

Plan of process.binding() deprecation #1482

Open
joyeecheung opened this issue Dec 12, 2023 · 4 comments
Open

Plan of process.binding() deprecation #1482

joyeecheung opened this issue Dec 12, 2023 · 4 comments

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Dec 12, 2023

This issue is not new but motivated by nodejs/node#50687, which proposed to

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

For background in 2021 the plan suggested in nodejs/node#37485 (review) and followed since nodejs/node#37819 was:

  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

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():

  1. What's our end-goal when the deprecation reaches EOL?
    a) Delete process.binding completely 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.
  2. How should the list of bindings (currently 20) be deprecated?
    a) Individually, based on how problematic they are/how difficult it is to provide alternatives
    b) Together at once
  3. Should the runtime deprecation start before or after alternatives/shims are completed
    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.
  4. How should the warnings be rolled out when we start to emit warnings (there are a few more combination possibilities with 2)
    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.

@targos
Copy link
Member

targos commented Dec 13, 2023

  1. I would advocate for a, with b and c as possible intermediary steps.
  2. 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.
  3. 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.
  4. Not sure non-node_module warnings would be useful in this case. Top-level apps probably don't use process.binding.

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 19, 2023

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

  1. TypeError: process.binding is not a function
  2. TypeError: process.binding(...).foo is not a function
  3. Error [ERR_UNSUPPORTED_OPERATION]: process.binding() is not supported anymore
  4. [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).

@jasnell
Copy link
Member

jasnell commented Dec 21, 2023

  1. I prefer (a) as the end result.
  2. I prefer (b) ... together at once
  3. Given my answers to 1 and 2, I prefer we land a runtime deprecation now (as per my PR)
  4. Just a regular runtime deprecation per my PR.

@davidmurdoch
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants