GeezerVison

LGTM, and really, thanks for doing this.

CI: https://ci.nodejs.org/job/node-test-commit/4653/

LGTM too.

LGTM once the comments above are fixed. One question, though — are there no tests that are affected by this?

This should also include a test case that ensures that the deprecation warning is emitted when expected

Btw, should this also hard-deprecate SlowBuffer() without a new?

Good point @ChALkeR ... it probably should. /cc @trevnorris

We could start filing some issues in advance.

Edited.

With that many occurrences, I feel we might need to do a longer deprecation cycle on this. Even with PRs, it could take awhile for patches to trickle down from deps of deps.

@Quard, that's only 1193 modules (approx), and I expect this list to look much less scary once they are ordered by the actual downloads/month stats. I will begin filing issues to the most important ones, see sidorares/node-mysql2#380 for an example.

@ChALkeR

Btw, should this also hard-deprecate SlowBuffer() without a new?

I don't see the point. As far as I'm aware, we're not planning to make SlowBuffer subclassable.

@jasnell

This should also include a test case that ensures that the deprecation warning is emitted when expected

Could you or someone else link to an existing test that does a similar thing?

@seishun Yes, I totally missed that, thanks.

Perhaps we should hard-deprecate SlowBuffer whatsoever in another PR, then. Not sure yet about the target on that one, though, but I guess 7.0.0 could be fine, as there are not so many SlowBuffer users and those of them who wish to support 0.12 and older v4.x versions could just use the shim. That should be discussed, though.

@seishun ... you can look at test/parallel/test-crypto-deprecated.js for an example of validating that the deprecation warning is emitted. It's pretty straightforward.

process.on('warning', (warning) => {
  assert.strictEqual(warning.name, 'DeprecationWarning');
  // etc...
});

SlowBuffer deprecation would affect only 444 packages.

Yeah, I am in favor of reverting. The reactions we are hearing from outside of Node core make it pretty clear that a runtime deprecation for functionality like this is just too disruptive, and we’re not going to get away with it, possibly ever.

Oh and since @Trott put this on the agenda (thanks!), ping @nodejs/ctc

The reactions we are hearing from outside of Node core make it pretty clear that a runtime deprecation for functionality like this is just too disruptive

Could you link to some of these reactions, because I'm only seeing negative comments from other Node.js collaborators?

@mafintosh @yoshuawuyts @substack have contributed to various wg's but afaik are not part of core

To put that in perspective, there were 1193 packages that would affected by this in August (#8169 (comment)). Evidently most of their developers don't mind fixing their code, or their users don't mind the warning, or there are no users.

@seisun how many are still a problem?

also 7 has only been out for a couple weeks, and does not have mass adoption yet... we can expect way more problems with 8, especially when it goes Lts

Maintained modules can be easily fixed. Unmaintained modules may have security issues or bugs, or they may simply be obsolete. So keeping old and insecure APIs just to keep unmaintained modules running will hurt the ecosystem long-term. Unmaintained things usually begin to decay, so building an ecosystem on them is not a great idea. Made by an unpaid volunteer or not, old code is old code.

If we want to hard-deprecate Buffer() and then eventually new Buffer(), then IMO we should do them both at the same time, and we should be clear about the reasoning: this is for security reasons. Buffer.alloc() is preferred over Buffer() since it zeroes out the memory.

It would be really unfortunate if folks started updating their code to use new just to have that get deprecated shortly thereafter.

So I've been following this thread, and what isn't clear to me is from the security point of view, why you can't you just zero-fill buffers by default? Specifically, make Buffer() and new Buffer() aliases for Buffer.from(). At worst that would cause old code that hasn't been updated to Buffer.allocUnsafe()to run slower.

I've been trying to think of a way forward on this that would allow us to maintain the existing uses of Buffer() with or without the use of new while still allowing us to progress with improved internals. There's one possible approach that I've come up with that's going to need a bit more investigation, but at first blush it's doable.

If we followed a pattern similar to:

class BufferArray extends Uint8Array {
  constructor(/*.. args ..*/) {
    /* ... */
  }
}

function Buffer(/* .. args ..*/) {
  if (!(this instanceof Buffer))
    return new Buffer(/* .. args ..*/);
  return Reflect.construct(BufferArray, [/** args **/], new.target);
}
util.inherits(Buffer, BufferArray);

Then calling Buffer() and new Buffer() as currently done would still continue to work and the resulting object would properly inherit from the new BufferArray ES6 class where the improved internal implementation would live. It would mean introducing a new object into the prototype chain, but it allows us a path forward that would enable existing code to keep functioning. Buffer itself essentially just becomes a backwards compatible shim for the new implementation.

There are obviously some details that would need to be worked out in this but, at the very least, it provides on possible option.

@jasnell have you seen addaleax/node@ec09d43? It goes pretty much into the direction you’re describing, with tests passing and everything, it would just take a bit of work to match the current performance (because it currently creates unnecessary intermediate objects).

(I’ve linked this before – in the other thread, I think – but I do see that these things can get a bit lost 😄)

I think I am +1 on reverting as well

@addaleax ... I think I saw the link at some point but I don't believe I'd actually seen the code. Will dig in more! Thank you!

@jasnell I am -1 on introducing an extra class. Things are already complicated enough with Array, Buffer, ArrayBuffer and UintXArray. Please try to imagine yourself in the shoes of someone who is just getting familiar with JavaScript and Node.js. Backward compatibility is important, but it would be unfortunate to permanently increase complexity in order to avoid a temporary disruption.

@jasnell I don't fully understand the Reflect thing but that seems like something that could work and not break things.

@seishun If we do this right, the additional complexity will be limited to something that can be explained in a single sentence; namely, the only difference between Buffer and BufferArray would be that BufferArray is that the latter is an ES6 class, meaning that it can’t be instantiated without new and that it can be inherited from.

And yeah, I’m not too keen on the name BufferArray (or my BufferClass) either, but I get that it’s hard to come up with a name for something that basically already exists under a different name. If nothing else works, I’d be for SubclassableBuffer, because that’s what it is.

I would like to ask everyone who has commented here to move the discussion to #9531, if only so the public discussion can take in a central place and not over multiple GH threads at once.