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

buffer: runtime-deprecate Buffer constructor #7152

Closed
wants to merge 4 commits into from

Conversation

@seishun
Copy link
Member

seishun commented Jun 4, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change
  1. New Buffer API was introduced to prevent accidental data leakage and/or DoS (see #4660). But there are older modules that haven't been updated and might still be vulnerable. Runtime deprecation encourages module authors to update to the new API, and the users to update their dependencies.
  2. It is desirable that Buffer be a proper ES6 class that can be extended. However, parts of the deprecated Buffer API (such as creating a Buffer instance without new) make it impossible, while other parts (such as construction from a string) make it more complex. It will make things much simpler if the problematic deprecated parts are removed. But they need to be runtime-deprecated first.
@ChALkeR
Copy link
Member

ChALkeR commented Jun 4, 2016

@seishun I don't think this is viable at the moment. New API hasn't been backported even to 4.x afaik.
They don't port not because the old API isn't hard depracated, but because the new API isn't supported everywhere yet.

There should be a single way to allocate Buffers on all supported node versions, so I hope that:

  1. The new API will get backported to 4.x soon.
  2. By the end of the year, when 0.10 and 0.12 get phased out, all supported Node.js versions would have the new Buffer API. Then, library writers can port to the new API without requiring third-party compatibility modules (or with them, if they care about 0.12 for some reason).
  3. When a significant part of libraries will migrate (this would mean that the ecosystem is mostly fine with dropping old versions) — hard-deprecation will be a good solution to give the remaining users an indication that they should also port.

Perhaps target for 8.0 and re-evaluate later?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 5, 2016

Btw, this should also probably hard-deprecate SlowBuffer at the same time — it was replaced by Buffer.allocUnsafeSlow.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

Yeah, this is something that's going to need to wait for a while... I'm thinking at least v8 but likely longer.

@Fishrock123
Copy link
Member

Fishrock123 commented Jun 6, 2016

I'm thinking V10+ at minimum. Usage of this is very widespread. We should evaluate as time goes on.

@seishun
Copy link
Member Author

seishun commented Jun 6, 2016

Usage of this is very widespread.

It's unlikely to decrease substantially without hard-deprecation. The whole point of hard-deprecation is to decrease the existing usage.

I think there might be a misunderstanding here. I would like to remind that hard-deprecation means a one-time console notification that can be disabled.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 6, 2016

@seishun At this point, users are not migrating to the new Buffer API not because they didn't get a deprecation message in every app, but because the new API isn't supported by our current LTS version (and the Maintenance versions).

@seishun
Copy link
Member Author

seishun commented Jun 6, 2016

@ChALkeR It's both. I'm not arguing against the lack-of-support-in-LTS factor.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 6, 2016

@Fishrock123 I still hope to see users migrating before v8.0 release. If the new API will be backported to v4, then I plan to file issues to most used repos starting from 2017-01, when 0.12 will become not supported anymore.

So hopefully by April 2017 the usage of old API will decrease, and we should at least re-evaluate the deprecation before v8.0 release.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 6, 2016

To be clear: I am not saying that we should definitely force this in v8.0, but we should postpone this at least until v8.0 (unless the new API will get backported to 0.10/0.12, which I find unlikely).

So, perhaps set a milestone to v8.0 to not forget to reevaluate by then?

@trevnorris
Copy link
Contributor

trevnorris commented Jun 7, 2016

This can never happen. Uint8Array methods will rely on the Buffer constructor (hence why the Buffer constructor has been changed to essentially become compatible with Uint8Array). Going forward the change that will need to be made is that Buffer will automatically zero-fill memory (again to match Uint8Array). We can even start removing functionality that isn't supported by Uint8Array, but the constructor itself will live on forever.

@seishun
Copy link
Member Author

seishun commented Jun 8, 2016

@trevnorris I don't follow. Uint8Array is a native JS feature, how can it rely on Buffer? Or do you mean methods added to it from Buffer.prototype? That doesn't rely on the constructor either.

Either way, I don't see how hard-deprecating the constructor is incompatible with the constructor living on forever.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2016

@seishun For some of the methods of Uint8Array that return typed arrays again, the .constructor property is inspected and used for creating of the return value:

> a = Buffer.alloc(4)
<Buffer 00 00 00 00>
> a.map === Uint8Array.prototype.map
true
> a.map(x => x+1)
<Buffer 01 01 01 01> // Not a Uint8Array! .map() called a.constructor, which is Buffer.

This is probably fixable by setting Buffer[Symbol.species] to something that does basically what the current Buffer constructor does, but that’s still behind the --harmony_species flag. (edit: “fixable” in the sense that one could still deprecate the Buffer constructor if that is desired).

@ChALkeR
Copy link
Member

ChALkeR commented Jun 8, 2016

@addaleax @trevnorris
Is that fixable by checking the argument type of the Buffer() and printing a hard-deprecation message only if argument is not a Buffer or a typed array?

@addaleax
Copy link
Member

addaleax commented Jun 8, 2016

@ChALkeR The .map() example will call new Buffer(n) via this line, so I don’t think that will work out.

@trevnorris
Copy link
Contributor

trevnorris commented Jun 8, 2016

@addaleax

This is probably fixable by setting Buffer[Symbol.species] to something that does basically what the current Buffer constructor does

I'm hoping to avoid that all together by making the Buffer constructor work correctly with class inheritance.

EDIT: so that basically any inherited methods will return a Buffer instance. e.g. buffer.subarray() now returns a Uint8Array instead of a Buffer. It would simply things if we could change Buffer to require using new. Then it can properly use class inheritance, and the whole thing would be solved.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2016

@trevnorris A couple of us chatted in IRC for a bit, and one of the problems would be that Buffer(n) without new wouldn’t work… but apparently setting Symbol.species is not even necessary:

$ node --harmony-species -p 'Buffer[Symbol.species] === Buffer'
true

So my comment above was not actually very meaningful.

@seishun
Copy link
Member Author

seishun commented Jun 16, 2016

@ChALkeR

To be clear: I am not saying that we should definitely force this in v8.0, but we should postpone this at least until v8.0 (unless the new API will get backported to 0.10/0.12, which I find unlikely).

Just 0.12, isn't it? Since support for 0.10 ends in October.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2016

@seishun

Just 0.12, isn't it? Since support for 0.10 ends in October.

Ah, yes. 0.10 EOL is at the same time when 7.0 is going to be released. But I really doubt that it would be feasible to backport to 0.12 which ends in December.

@seishun seishun force-pushed the seishun:deprecate-buffer branch 2 times, most recently Jun 18, 2016
@seishun
Copy link
Member Author

seishun commented Jun 18, 2016

After some IRC discussion, it seems there is some agreement that the buffer shim would be sufficient for module authors who want to support old Node.js versions. So perhaps there might be no need to delay this by another 6 months.

Also, I've found a way to hard-deprecate the Buffer constructor without affecting Uint8Array methods that doesn't depend on @@species (see last commit). Its side effect is that Buffer == buf.constructor no longer holds, but it doesn't break any tests and it seems unlikely that a lot of people rely on it.

I've also updated the OP with another reason for hard-deprecation - namely, that it's a necessary step before Buffer can become a proper class that users can extend.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 19, 2016

@seishun

After some IRC discussion, it seems there is some agreement that the buffer shim would be sufficient for module authors who want to support old Node.js versions.

Actually, on a second thought I'm fine with that, but I still want this backported to v4. I will make a PR for that to start the discussion.

Also, I've found a way to hard-deprecate the Buffer constructor without affecting Uint8Array methods that doesn't depend on @@species (see last commit). Its side effect is that Buffer == buf.constructor no longer holds, but it doesn't break any tests and it seems unlikely that a lot of people rely on it.

See https://gist.github.com/ChALkeR/41cd82a8f3b8822860c77116e880d3c1. bson module does that in one line, could that change break things for someone who uses the bson module?

@@ -94,17 +99,19 @@ function alignPool() {
}
}

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

This comment has been minimized.

Copy link
@ChALkeR

ChALkeR Jun 19, 2016

Member

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.

This comment has been minimized.

Copy link
@seishun

seishun Jun 19, 2016

Author Member

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

* 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

This comment has been minimized.

Copy link
@ChALkeR

ChALkeR Jun 19, 2016

Member

Ditto.

This comment has been minimized.

Copy link
@seishun

seishun Jun 19, 2016

Author Member

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

@seishun
Copy link
Member Author

seishun commented Jun 19, 2016

See https://gist.github.com/ChALkeR/41cd82a8f3b8822860c77116e880d3c1. bson module does that in one line, could that change break things for someone who uses the bson module?

Well, I just ran bson's tests with this change and got 1 unrelated failure ("global var leak detected"), same as on current node. It did throw a bunch of Buffer deprecation warnings though.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 19, 2016

@seishun I'm not sure if relying on the tests is sufficient here, it looks like they don't calculate the coverage. Not sure if that code is actually used somewhere, though.

The file in question is https://github.com/mongodb/js-bson/blob/master/alternate_parsers/faster_bson.js#L369, and it looks like it's only used by some benchmark in that repo. I couldn't find any mentions of faster_bson in any other packages, btw.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 19, 2016

@seishun #7169 landed with allocUnsafeSlow for v5.x, preparing a non-intrusive PR for v4.x.

@mafintosh
Copy link
Member

mafintosh commented Nov 7, 2016

This isn't about supporting 0.10 or older versions of 4 - a choice that should be completely up to the module authors btw. This is about deprecating a substantial amount of modules on npm and basically showing that Node core doesn't take backwards compatibility seriously.

This isn't a change that should be postponed to v10. This is a change that should never happen and core should acknowledge that.

@ChALkeR
Copy link
Member

ChALkeR commented Nov 7, 2016

@mafintosh, everyone, I have counterarguments to that, but they are not going to fit inside a comment. I am preparing a longer explanation, and it's going to be ready in a few days. I believe that we could continue this conversation then.

@kgryte
Copy link

kgryte commented Nov 8, 2016

This should never land. The primary focus for core should be stability above all else. This was part of the Node ethos which has been lost since the end of the Great Node/io.js Schism.

Because of several changes in Node core, I have been forced to spend many hours needlessly updating packages and code which I should not have to be updating, simply because someone wanted core to use some (and in almost all cases, unnecessary) ES2015+ feature.

The current predominant attitude by many contributing to Node core seems to be that breaking things is fine as long as tagged with a semver major. That is an okay attitude for userland packages, but not in the platform itself. What I, as a package author, want more than anything else is stability. Period.

In a way, semver has become weaponized, a cudgel used to "force users to update". I have seen this happen repeatedly in Node core and as embodied by this PR.

And frankly, because of the focus on supposed "innovation" and updating core with the "latest" and "greatest", I have lost trust in core, as I cannot trust that Node will provide a stable API. As a result, I try to avoid using core functionality and, where possible, would rather just write my own implementation over which I have full control.

And if Node core development keeps going down the "breaking stuff is fine" path, I foresee a future where many people will adopt a similar attitude to myself and avoid core altogether. I cannot see how Node can perceive this as desirable.

In short, if you want the ability to subclass, create a new API. Stability is not something which should be flippantly discarded.

@bnoordhuis
Copy link
Member

bnoordhuis commented Nov 8, 2016

The primary focus for core should be stability above all else. This was part of the original philosophy of Node

Hah, no. Stability wasn't an overriding concern until node was already several years old. We didn't get serious about that until v0.8/v0.10.

I get that you feel stability should come first but you are committing the cardinal sin of rewriting history to fit your narrative.

@jasnell jasnell added the blocked label Nov 8, 2016
@jasnell
Copy link
Member

jasnell commented Nov 8, 2016

Let me try again to make the status of this PR clear: There is no consensus to land this PR at the current time. The CTC discussed the matter several months ago now and decided that a runtime deprecation of the Buffer() constructor is not feasible or desirable given the ramifications to the ecosystem. For those of you who have weighed in saying that this should never land, trust that we have heard and noted your input and if and when this PR comes back up for consideration by the CTC again, we will absolutely be taking those considerations into account. It does absolutely no good whatsoever to rehash those comments or to demand that the CTC favor stability of all other considerations. Let me be even clearer: there was absolutely no one on the CTC who voiced the opinion that this PR should land at any point in the foreseeable future.

That said, we are not closing the PR because there is a possibility that we might decide to go down this route. Why would we do so? The only reason why would intentionally break the Buffer() constructor would be if it was absolutely certain that doing so would be in the larger ecosystems best interest -- which would be determined by evaluating the possible security, usability, and performance benefits of such a change weighed against the potential ecosystem impact.

Understand that those of us on the CTC are also Node.js users. These kinds of breaking changes affect us also. We are not keen on breaking our own code, much of which depends directly on modules that would be directly impacted by these changes. We do not make decisions to break APIs lightly.

@Fishrock123
Copy link
Member

Fishrock123 commented Nov 8, 2016

fwiw I still think this should be a goal in the long run

@addaleax
Copy link
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.

@MylesBorins MylesBorins force-pushed the nodejs:master branch to 54fef67 Feb 1, 2017
@seishun seishun changed the title buffer: hard-deprecate Buffer constructor buffer: runtime-deprecate Buffer constructor Feb 16, 2017
@jasnell jasnell added the stalled label Mar 1, 2017
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

@nodejs/ctc ... should we keep this open? I'm pretty sure the discussion has run it's course here.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 2, 2017

@jasnell, I think that keeping a PR for this open makes sense, at least until we decide against that. The actual discussion is in #9531 now, but the PR is valuable by itself. So, -1 to closing this atm.

const bindingObj = {};
const internalUtil = require('internal/util');

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

This comment has been minimized.

Copy link
@ChALkeR

ChALkeR Mar 12, 2017

Member

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

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

This comment has been minimized.

Copy link
@seishun

seishun Mar 12, 2017

Author Member

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

This comment has been minimized.

Copy link
@ChALkeR

ChALkeR Mar 12, 2017

Member

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

This comment has been minimized.

Copy link
@ChALkeR

ChALkeR Mar 12, 2017

Member

/cc @jasnell

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 12, 2017

Member

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

@ChALkeR
Copy link
Member

ChALkeR commented May 4, 2017

This was mostly superseded by #11968, and there is no concrete path forward runtime-deprecating it without a flag atm, and probably in another half a year.
Also, the PR would have to look different since #11968 landed, and the most important bits (those that make sure that typed arrays aren't broken) have been incorporated there.

Perhaps it's time to close this and revisit later, probably in another PR. /ping @jasnell — could you confirm?

@seishun thanks on the work here! It was important, and helped to reach the current state of runtime-deprecated-under-a-flag state of things.

@jasnell
Copy link
Member

jasnell commented May 4, 2017

Yeah, I think this should just be closed at this point. We can revisit it later.

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

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.