TSC icon indicating copy to clipboard operation
TSC copied to clipboard

Plan of process.binding() deprecation

Open joyeecheung opened this issue 1 year ago • 13 comments

This issue is not new but motivated by https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/pull/37485#pullrequestreview-600060802 and followed since https://github.com/nodejs/node/pull/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.

joyeecheung avatar Dec 12 '23 16:12 joyeecheung

  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.

targos avatar Dec 13 '23 10:12 targos

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).

joyeecheung avatar Dec 19 '23 17:12 joyeecheung

  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.

jasnell avatar Dec 21 '23 02:12 jasnell

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!

davidmurdoch avatar Mar 26 '24 20:03 davidmurdoch