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

buffer: hard-deprecate calling Buffer without new #8169

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@seishun
Member

seishun commented Aug 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Light version of #7152.

We want to make Buffer a class so that it can be subclassed. However, instantiating a class requires new. This hard-deprecates calling Buffer without new.

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 18, 2016

LGTM, and really, thanks for doing this.

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 18, 2016

@Qard

This comment has been minimized.

Member

Qard commented Aug 18, 2016

LGTM too.

@seishun seishun referenced this pull request Aug 18, 2016

Closed

buffer: runtime-deprecate Buffer constructor #7152

2 of 2 tasks complete
@@ -65,6 +71,9 @@ function alignPool() {
* would ever actually be removed.
**/
function Buffer(arg, encodingOrOffset, length) {
if (!(this instanceof Buffer)) {

This comment has been minimized.

@jasnell

jasnell Aug 18, 2016

Member

Perhaps new.target instead? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target

if (!new.target) {
  // print deprecation warning
}
@@ -65,6 +71,9 @@ function alignPool() {
* would ever actually be removed.
**/
function Buffer(arg, encodingOrOffset, length) {
if (!(this instanceof Buffer)) {
bufferDeprecationWarning();
}

This comment has been minimized.

@jasnell

jasnell Aug 18, 2016

Member

Instead of this, please use process.emitWarning() ...

process.emitWarning(
  'Calling Buffer() without `new` is deprecated. Use Buffer.alloc(), ' +
  'Buffer.allocUnsafe(), Buffer.from(), or new Buffer() instead.',
  'DeprecationWarning'
);

This comment has been minimized.

@addaleax

addaleax Aug 18, 2016

Member

@jasnell That would emit a warning every time Buffer() is called, not only the first time, right?

This comment has been minimized.

@jasnell

jasnell Aug 18, 2016

Member

right.. there should be a gate... like so:

var newBufferWarned = false;
function Buffer(arg, encodingOrOffset, length) {
  if (!new.target && !newBufferWarned) {
    newBufferWarned = true;
    process.emitWarning(
      'Calling Buffer() without `new` is deprecated. Use Buffer.alloc(), ' +
      'Buffer.allocUnsafe(), Buffer.from(), or new Buffer() instead.',
      'DeprecationWarning'
    );
  }
  //...

This comment has been minimized.

@seishun

seishun Aug 19, 2016

Member

Could you clarify the purpose of such change? It would introduce needless boilerplate and make it inconsistent with the rest of node.

This comment has been minimized.

@jasnell

jasnell Aug 19, 2016

Member

See #8166 ... I'm working towards making this more consistent. If you look at the current implementation of the internal/util printDeprecationMessage() you'll see that it simply defers to the process.emitWarning() method already, so it's a bit of an unnecessary indirection.

Also, the way that bufferDeprecationWarning() is implemented in this PR is unnecessarily more complex than it needs to be. There's no reason to create a new object and closure.

This comment has been minimized.

@seishun

seishun Aug 19, 2016

Member

@jasnell Done. I kept the wording because "Calling Buffer() without new is deprecated." implies that calling Buffer with new is not deprecated.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 18, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 18, 2016

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

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 18, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 18, 2016

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

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 18, 2016

We could start filing some issues in advance.

Edited.

@Qard

This comment has been minimized.

Member

Qard commented Aug 18, 2016

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.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 18, 2016

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

@seishun

This comment has been minimized.

Member

seishun commented Aug 19, 2016

@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?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 19, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 19, 2016

@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...
});
const bufferDeprecationWarning =
deprecate(() => {}, 'Using Buffer without `new` will soon stop working. ' +
'Use `new Buffer`, or preferably ' +
'Buffer.from, Buffer.allocUnsafe or Buffer.alloc instead.');

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 19, 2016

Member

probably do not mention allocUnsafe?

if people need that they will find it I think :)

newBufferWarned = true;
process.emitWarning(
'Using Buffer without `new` will soon stop working. ' +
'Use `new Buffer`, or preferably ' +

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 19, 2016

Member

Actually since this will be in v7... maybe we should only recommend Buffer.from() and Buffer.alloc() (pref with the parenthesis?)

This comment has been minimized.

@jasnell

jasnell Aug 19, 2016

Member

That works for me misread the comment... see #8169 (comment) instead.

process.emitWarning(
'Using Buffer without `new` will soon stop working. ' +
'Use `new Buffer`, or preferably ' +
'Buffer.from, Buffer.allocUnsafe or Buffer.alloc instead.',

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 19, 2016

Member

since my previous comment got squashed... maybe without allocUnsafe?

I think the people who need that will be able to find it.

This comment has been minimized.

@seishun

seishun Aug 19, 2016

Member

This might backfire. People might be using Buffer() in performance-critical code. They might try Buffer.alloc in a hurry, see that it slows down their code, then shrug and just use new Buffer().

This comment has been minimized.

@jasnell

jasnell Aug 19, 2016

Member

I would keep all three listed and just drop the 'Use new Buffer'... just point to the three new constructors in the order: Buffer.from(), Buffer.alloc(), and Buffer.from().

Also, a small nit: @seishun, can you add the () after each of the functions?

This comment has been minimized.

@seishun

seishun Aug 19, 2016

Member

I imagine users who don't keep track of Node.js development would be thoroughly confused by a message that says they shouldn't call Buffer without new and then tells them to use something entirely different. They would wonder whether they can just use new Buffer and if not, why they don't get such a message with new Buffer.

The current message at least gives them the choice to either read the documentation and learn about the new APIs, or do the quick fix if they're in a hurry.

This comment has been minimized.

@ChALkeR

ChALkeR Aug 19, 2016

Member

@jasnell What order did you mean? As there is clearly a mistype somewhere. =)

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 19, 2016

SlowBuffer deprecation would affect only 444 packages.

@Trott Trott removed the ctc-review label Nov 3, 2016

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 4, 2016

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.

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 4, 2016

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

@seishun

This comment has been minimized.

Member

seishun commented Nov 4, 2016

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?

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 4, 2016

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

@seishun

This comment has been minimized.

Member

seishun commented Nov 4, 2016

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.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 4, 2016

@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

@alex7kom

This comment has been minimized.

alex7kom commented Nov 4, 2016

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.

@feross

This comment has been minimized.

Contributor

feross commented Nov 4, 2016

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.

@wmhilton

This comment has been minimized.

wmhilton commented Nov 7, 2016

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.

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 7, 2016

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.

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 7, 2016

@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 😄)

@evanlucas

This comment has been minimized.

Member

evanlucas commented Nov 7, 2016

I think I am +1 on reverting as well

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 7, 2016

@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!

@seishun

This comment has been minimized.

Member

seishun commented Nov 8, 2016

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

@mafintosh

This comment has been minimized.

Member

mafintosh commented Nov 8, 2016

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

@addaleax

This comment has been minimized.

Member

addaleax commented Nov 8, 2016

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

addaleax added a commit to addaleax/node that referenced this pull request Nov 9, 2016

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(nodejs#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.
@addaleax

This comment has been minimized.

Member

addaleax commented Nov 9, 2016

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.

@Trott Trott removed the ctc-agenda label Nov 16, 2016

evanlucas added a commit that referenced this pull request Nov 28, 2016

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.

PR-URL: #9529
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

addaleax added a commit that referenced this pull request Dec 5, 2016

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.

PR-URL: #9529
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@seishun seishun deleted the seishun:new branch Aug 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment