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.

Sign up for GitHub

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: runtime-deprecate Buffer constructor #7152

Closed
wants to merge 4 commits into from
Closed

buffer: runtime-deprecate Buffer constructor #7152

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e7fbcf
buffer: hard-deprecate Buffer constructor
seishun Jun 3, 2016
1093f3c
Make sure Uint8Array methods are not affected by the deprecation
seishun Jun 18, 2016
e11e640
simplify things
seishun Aug 7, 2016
c614975
revert unnecessary change
seishun Aug 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@

const binding = process.binding('buffer');
const { isArrayBuffer } = process.binding('util');
const { deprecate } = require('internal/util');
const bindingObj = {};
const internalUtil = require('internal/util');

class FastBuffer extends Uint8Array {}
const FastBuffer = (() => class Buffer extends Uint8Array {})();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind why this change was needed? Which code breaks without it?

#11808 and #11806 are missing this, would everything be fine there?

Copy link
Contributor Author

@seishun seishun Mar 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChALkeR it was necessary because the FastBuffer.prototype.constructor = Buffer; line was removed. It was removed to prevent Uint8Array methods (which call the constructor) from being affected by the deprecation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, here is the testcase: Buffer.alloc(10).map(x => x + 1) (just for future reference).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @jasnell

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd been trying to remember where I had seen this :-)


FastBuffer.prototype.constructor = Buffer;
Buffer.prototype = FastBuffer.prototype;

exports.Buffer = Buffer;
Expand Down Expand Up @@ -54,17 +54,19 @@ function alignPool() {
}
}

const bufferDeprecationWarning =
deprecate(() => {}, 'Buffer constructor is deprecated. ' +
'Use Buffer.from, Buffer.allocUnsafe or Buffer.alloc instead.');
Copy link
Member

@ChALkeR ChALkeR Jun 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the order here to be Buffer.from, Buffer.alloc or Buffer.allocUnsafe instead, because the latter should be used only when people are better aware of what they are doing, i.e. are sure that they don't leak the resulted Buffer in an (even partially) unitialized state in any code branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied from the comment below, but I don't mind changing the order.


/**
* The Buffer() construtor is "soft deprecated" ... that is, it is deprecated
* in the documentation and should not be used moving forward. Rather,
* developers should use one of the three new factory APIs: Buffer.from(),
* Buffer.allocUnsafe() or Buffer.alloc() based on their specific needs. There
* is no hard deprecation because of the extent to which the Buffer constructor
* is used in the ecosystem currently -- a hard deprecation would introduce too
* much breakage at this time. It's not likely that the Buffer constructors
* would ever actually be removed.
* The Buffer() construtor is deprecated ... that is, it should not be used
* moving forward. Rather, developers should use one of the three new factory
* APIs: Buffer.from(), Buffer.allocUnsafe() or Buffer.alloc() based on their
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

@seishun seishun Jun 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking that this comment doesn't really serve any useful purpose anymore and can be removed.

* specific needs. It's not likely that the Buffer constructors would ever
* actually be removed.
**/
function Buffer(arg, encodingOrOffset, length) {
bufferDeprecationWarning();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this conditional, leaving it only for the cases in which Buffer() is invoked without new?

Copy link
Contributor Author

@seishun seishun Aug 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I don't think we should.

  • If we hard-deprecate new Buffer(string, encoding) now, we'll have an option to remove it later. Implementing it with the same behavior in class Buffer requires a significant amount of boilerplate code.
  • new Buffer(int) needs to stay for compatibility with Uint8Array, but hard-deprecating it gives us leeway to change its behavior. Currently, new Buffer(int) returns non-zeroed and pooled memory (fast), but new Uint8Array(int) returns zeroed and non-pooled memory (slow). If new Buffer(int) is hard-deprecated for 6 months, it's likely that much fewer people would be affected by the sudden performance drop if we changed Buffer to inherit from Uint8Array without overriding the constructor (basically just class Buffer extends Uint8Array {}).

Again, hard-deprecation doesn't necessarily mean removing it later, but it at least allows us to consider it.

And in any case, the Buffer constructor is already soft-deprecated, which itself is a reason for hard-deprecation (see the first point in OP).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, these are some valid points. I am actually quite sure there’s no reason to change the semantics of new Buffer(), with all combinations of arguments, but I should probably write a proof of concept for that before making definitive statements. ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seishun addaleax@ec09d43 – Probably not the most performant way of doing anything, but it provides users with a subclassable BufferClass export that works exactly like the current Buffer constructor (all tests passing).

If the goal is to make Buffer itself subclassable, then one could work towards such a solution that would ultimately alias Buffer to what BufferClass is in my suggestion. That would break Buffer() without new but leave everything else intact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I don't see any immediate issues with this.

However, the question is whether we want to maintain all this code just to support a deprecated feature that has an equivalent alternative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a purely technical viewpoint I completely agree with you, and pushing users towards Buffer.{from,alloc*} certainly is a good idea. But to be honest, I’m a bit scared of the possible amount of breakage that would come from removing functionality from the Buffer constructor…

@ChALkeR Could you try and grep for modules that currently use Buffer() without new? That might give a concrete idea of how huge/manageable removing that would be, even if it happens way into the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seishun

As far as I know, implementing it with the same behavior in class Buffer is quite complicated, if possible at all.

I was wrong on this. All we'd need to do is something like:

class Buffer extends Uint8Array {
  constructor(string, encoding)  {
    const ab = binding.newArrayBufferFromString(string, encoding);
    super(ab);
  }
}

@addaleax There have been a couple suggestions of exporting a Buffer class that can be inherited, but it always wraps back around that it should be able to extend Buffer directly. For cleanliness of API I tend to agree, even if that involves breaking an egg or two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be honest, I’m a bit scared of the possible amount of breakage that would come from removing functionality from the Buffer constructor…

Exactly, which is why we should hard-deprecate first. In the 6 months that follow, we should be able to evaluate the feasibility of removing this functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point… I’d still prefer this to be restricted to the new-less variant but I think you generally convinced me that this is a good idea. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just back from vacation and suspect that missed lots of conversations, but generally I played with subclassing Buffer as was suggested in another thread using Reflect, and it introduces up to ~3x slowdown on buffer creation in some cases (if we want to preserve new-less variant), so for now trying to figure out if there is a better way.

// Common case.
if (typeof arg === 'number') {
if (typeof encodingOrOffset === 'string') {
Expand Down