buffer: do deprecation warning outside `node_modules` #19524

addaleax · 2018-03-22T00:20:15Z

In addition to --pending-deprecation, emit a deprecation warning
for usage of the Buffer() constructor for call sites that are outside
of node_modules.

The goal of this is to better target developers, rather than
burdening users with an omnipresent and quickly ignored warning.

This implements the result of a TSC meeting discussion
from March 22, 2018.

Refs: #19079 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

fyi @nodejs/collaborators

(Fwiw, I would still prefer to not do this and/or wait until after the Node 10 release.)

addaleax · 2018-03-22T00:22:00Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/13817/~~
CI: https://ci.nodejs.org/job/node-test-commit/17093/

vsemozhetbyt · 2018-03-22T00:34:42Z

doc/api/deprecations.md

@@ -93,6 +93,10 @@ is strongly recommended:
 * [`Buffer.from(string[, encoding])`][from_string_encoding] - Create a `Buffer`
  that copies `string`.

+As of REPLACEME, a deprecation warnings is printed at runtime, when


Nit: "warning is" or "warnings are"?

ChALkeR · 2018-03-22T00:56:59Z

doc/api/buffer.md

@@ -315,6 +315,10 @@ It can be constructed in a variety of ways.
 <!-- YAML
 deprecated: v6.0.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/???


This can now be updated to 19524 (here and below).

ChALkeR · 2018-03-22T01:03:23Z

lib/internal/util.js

+  const { runInNewContext } = require('vm');
+  // Use `runInNewContext()` to get something tamper-proof and side-effect-free.
+  // Since this is currently only used for a deprecated API, at most once
+  // per process, the perf implications should be okay.


The problem here is that this is not called once per process. It's called once per every new Buffer() call until it finds a one that is not inside node_process.

How long would 1k / 10k calls take?
Perhaps, bail out after 1k/10k per process if no other suggestions?

Right – I switched to caching the function now.

I get ~ 16µs per call for this, so 10k calls would be 0.16 seconds.

(That’s a lot, yes.)

seishun

The goal of this is to better target developers, rather than burdening users with an omnipresent and quickly ignored warning.

There are many problems with this statement.

We decided to deprecate the Buffer constructor because it's problematic, and the arguments for that are relevant regardless of where the code is located. Not showing the warning for call sites inside node_modules means hiding a (likely large) part of the problem.
There is no evidence that a single-time warning is a significant burden for users, or that they "quickly ignore" it. At least I haven't seen such evidence having closely followed every Buffer-deprecation-related issue.
Many modules are not actively tested on the latest Node.js versions, which means the problematic code in them will never be fixed, since no users will see and report the warning.
Many applications and modules depend on old module versions, which means they'll continue running with problematic code even if all dependencies have been fixed long ago.

In addition, this behavior is surprising and inconsistent with other deprecation warnings. Imagine you're using Node.js for the first time, you copy some Buffer code from a module you're using (that doesn't print any warnings!) into your own code, and suddenly you get a warning. Then you spend a lot of time figuring out why it was not printed before, only to learn that it depends on the path to the file. That's not a great first impression.

Trott · 2018-03-22T01:43:02Z

@seishun You make some valid points. I'm only responding to the ones that I think could benefit from a broader context. I don't expect this to persuade you. Just informing the larger discussion.

There is no evidence that a single-time warning is a significant burden for users, or that they "quickly ignore" it. At least I haven't seen such evidence having closely followed every Buffer-deprecation-related issue.

As one of several people who have been running with export NODE_PENDING_DEPRECATION=1 set in my shell rc file, I can attest that the warnings end up everywhere for CLI tools. It is reasonable to conclude that most users will learn to ignore these messages completely.

Many modules are not actively tested on the latest Node.js versions, which means the problematic code in them will never be fixed, since no users will see and report the warning.

They will (hopefully) get fixed when the warnings are enabled for the contents of node_modules which will presumably happen in a later release.

ChALkeR · 2018-03-22T04:31:45Z

lib/internal/util.js

+      Error.prepareStackTrace = function(err, trace) {
+        err.stack = trace;
+      };
+      Error.stackTraceLimit = Infinity;


Infinity? Why? That would mean that a function that does new Buffer in a callback of e.g. express would not get the warning, correct? Shouldn't this check just the location of the file that does new Buffer()?

Ah, disregard this one comment, I think I understand how this works now, sorry =).
Not deleting just to be double-sure.

ChALkeR · 2018-03-22T04:32:25Z

lib/internal/util.js

+    const { runInNewContext } = require('vm');
+    // Use `runInNewContext()` to get something tamper-proof and
+    // side-effect-free. Since this is currently only used for a deprecated API,
+    // the perf implications should be okay.


I still suggest bailing out after 10k attempts (which are about ~0.1 second).

@ChALkeR I think it might be helpful if you could explain why you are both very much pro-deprecation and in favor of moving all code off the deprecated API, but at the same time want to keep the performance on the same level as it was before?

Like @mafintosh said, isn't the perf hit a good incentive to move away?

@addaleax Because, according to the numbers you presented, this change has the potential of silently adding from 10x to 100x synchronous performance hit on some workloads (for buffers of size 1 KiB), which could affect streams and stuff like that significantly.

Presenting that performance hit to people without any warning (so they have no idea what's going on) could be pretty disruptive and can cause issues with real deployments out there.

In my opinion, a deprecation warning would be less disruptive than a 100x silent performance hit.

lpinca · 2018-03-22T06:18:50Z

They will (hopefully) get fixed when the warnings are enabled for the contents of node_modules which will presumably happen in a later release.

I'll repeat myself but in my opinion if we don't bug users and module authors in a way or another, nothing will be fixed and we'll just keep postponing the problem.

mscdex · 2018-03-22T06:19:31Z

I agree with @lpinca

bmeck · 2018-03-22T08:57:01Z

I have some concerns, we don't always develop our application code in a single top level package, we put various bootstrapping inside of node_modules including our server bootstrap. For cases where we have something like:

/app-logic
/app-logic/node_modules/server

Wouldn't this prevent deprecations coming from server which we update as a peer dependency essentially?

Also, wouldn't this cause problems if I had a parent directory containing node_modules such as /usr/local/lib/node_modules/ which is the default npm install -g directory on my machine? I would think running executables would be the most important place to give such deprecation.

MylesBorins · 2018-03-22T15:20:18Z

@mafintosh Would the top level completely solve your problems? One thought I have is that if we still warn for top level modules, individual module test suites could still be broken by this. If the suites are broken by the warning, they will not be able to actively test against the latest version.

That being said, this approach is likely to have less people opening issues against the modules due to the warnings, meaning that the errors could be fixed when they are encountered... which is far less disruptive.

Thoughts?

mafintosh · 2018-03-22T15:27:41Z

@MylesBorins I think I'm 👎. The main thing I see this solving is stopping people writing new modules with new Buffer. As I understand it, that was one of the main TSC reasons for doing the runtime deprecation now. With the approach taken as is, in this PR, users writing new modules with new Buffer will immediately get hit by the deprecation warning (I'm open to even start throwing here at some point!). I think we should give this approach a chance to work. It's not like userland isn't updating as it is, it's just a very slow and looooong process.

mafintosh · 2018-03-22T15:29:11Z

In addition the approach taken by @addaleax here has the added "benefit" of slowing down code running with new Buffer, which could be seen as an extra carrot for users to upgrade.

MylesBorins · 2018-03-22T15:31:04Z

@mafintosh did you mean to 👍🏽?

To be more explicit, are you in support of this approach? I personally think it is a good idea, but just wanted to confirm that the above concern wasn't an issue

mafintosh · 2018-03-22T16:00:12Z

@MylesBorins hehe sorry, I thought you asked me about @bmeck's comment. I'm 👍 on @addaleax's PR. I actually suggested this approach in the Buffer runtime thread

addaleax · 2018-03-22T22:07:29Z

Also, wouldn't this cause problems if I had a parent directory containing node_modules such as /usr/local/lib/node_modules/ which is the default npm install -g directory on my machine? I would think running executables would be the most important place to give such deprecation.

@bmeck Yes. For the npm install -g use case, this is intended (at least by me) -- CLIs are almost never vulnerable to the actual issue, and since we're not talking about "real" breakage of a feature, there doesn't seem to be a point in warning users about it.

Regarding the other condition, intentionally calling a directory node_modules as part of code management: No idea what to do about it, tbh. I kind of hope that it's rare enough to not matter in the big picture of things.

mafintosh · 2018-03-22T22:16:11Z

lib/internal/util.js

+    // it's likely from Node.js core.
+    if (!/^\/|\\/.test(filename))
+      continue;
+    return /[\\/]node_modules[\\/]/.test(filename);


if require.main.filename contains node_modules, we should ignore that node_modules folder here. I know plenty of devs (including myself), that use a node_modules folder to contain all the dev code to make cross module development easier.

Something like:

const nodeModulesParent = require.main.filename.indexOf('/node_modules/') // add windows support function isNodeModules (filename) { const i = filename.lastIndexOf('/node_modules/') return i !== -1 && i > nodeModulesParent }

This has the added benefit of making deprecations show if you cd into node_modules and run a test fx (i do that a bunch as well)

@mafintosh thanks, i'll look into it later, this makes sense!

I think we might adjust it abit because there might be different prefixes before /node_modules/ and we want to take care of backlashes as well, but generally this sgtm

@mafintosh updated, please take a look!

benjamingr · 2018-03-22T22:44:28Z

lib/internal/util.js

+  for (const frame of stack) {
+    const filename = frame.getFileName();
+    // If a filename does not start with / or contain \,
+    // it's likely from Node.js core.


Can you explain "it's likely"?

@benjamingr You could do the same thing we're doing here, run something inside a vm context with a specified filename.

If there are more such edge cases, I'm not aware of those, but I'm not comfortable claiming there aren't any either :)

jdalton · 2018-03-25T16:44:12Z

lib/internal/util.js

+  // Match the prefix of the main file, if it is inside a `node_modules` folder,
+  // up to and including the innermost `/node_modules/` bit, so that
+  // we can later ignore files which are at the same node_modules level.
+  if (mainPrefix === undefined) {


👆 I little e.g. would help. like

e.g. `a/node_modules/b/node_modules/c/node_modules/d/d.js` would match `a/node_modules/b/node_modules/c/node_modules/`

rvagg · 2018-04-03T00:35:42Z

Yeah, I think I can dig this, I like the 10k cutoff as a way to lean-in given the heavy-handed way the /node_modules/ check is done. Gives us another dial to turn to increase the pressure more slowly than a binary deprecation. It'll be interesting to see if this causes a hit on microbenchmarks that people will inevitably apply to Node 10 when it goes live. 🤞 that TF+I improvements give us an equivalent boost to make it disappear in the general case.

mcollina

LGTM

Trott · 2018-04-03T17:23:34Z

Given @seishun's objections in #19524 (review) and assuming they have not been addressed/answered, this cannot land without a TSC vote. So I'm going to put this on the agenda for tomorrow. If we're going to do this, we should decide that promptly. If we are not going to do it, we should close this and figure out what we are going to do.

BridgeAR · 2018-04-03T17:34:59Z

@seishun I know you want to have a stricter version than the suggestion here. Instead of the TSC vote, do you not maybe want to accept this PR and open a own PR afterwards to make the deprecation stricter? I suggest that mainly out of fear that we might end up with no deprecation message at all in 10.x if nothing lands soon :/.

seishun · 2018-04-03T17:47:41Z

@BridgeAR Judging from the TSC meeting discussion from March 22, 2018, the only reason why TSC might reject this PR (which seems unlikely given 5 approvals on behalf of TSC) is if they decide to land #15346 instead. So I would like to keep my objection explicit.

But I'll be sure to update #15346 accordingly once/if this lands. :)

mhdawson

LGTM

addaleax · 2018-04-04T10:39:32Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1349/
CI: https://ci.nodejs.org/job/node-test-commit/17417/

MylesBorins

Rubber Stamp LGTM

Trott · 2018-04-04T17:14:43Z

TSC vote in the meeting today:

YES (10): @addaleax @Trott @targos @cjihrig @evanlucas @MylesBorins @mcollina @gibfahn @thefourtheye @ofrobots
NO (0)
ABSTAIN (0)

10 is enough to approve (19 members, so that's 50% + 1), so this can land.

In addition to `--pending-deprecation`, emit a deprecation warning for usage of the `Buffer()` constructor for call sites that are outside of `node_modules`. The goal of this is to better target developers, rather than burdening users with an omnipresent and quickly ignored warning. This implements the result of a TSC meeting discussion from March 22, 2018. Refs: nodejs#19079 (comment) PR-URL: nodejs#19524 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

In addition to `--pending-deprecation`, emit a deprecation warning for usage of the `Buffer()` constructor for call sites that are outside of `node_modules`. The goal of this is to better target developers, rather than burdening users with an omnipresent and quickly ignored warning. This implements the result of a TSC meeting discussion from March 22, 2018. PR-URL: nodejs#19524 Refs: nodejs#19079 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

BridgeAR · 2018-04-09T22:45:35Z

Landed in 9d4ab90 🎉 🎉 🎉

Taking the source code of a function and running it in another context does not play well with coverage instrumentation. For now, do the simple thing and just write the source code as a string literal. Fixes: nodejs#19912 Refs: nodejs#19524

Taking the source code of a function and running it in another context does not play well with coverage instrumentation. For now, do the simple thing and just write the source code as a string literal. Fixes: #19912 Refs: #19524 PR-URL: #20035 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

addaleax added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 22, 2018

addaleax added this to the 10.0.0 milestone Mar 22, 2018

addaleax requested review from seishun and a team March 22, 2018 00:20

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Mar 22, 2018

addaleax added notable-change PRs with changes that should be highlighted in changelogs. and removed util Issues and PRs related to the built-in util module. labels Mar 22, 2018

addaleax force-pushed the buffer-node-module branch from 78bddf0 to 7fc297c Compare March 22, 2018 00:25

vsemozhetbyt reviewed Mar 22, 2018

View reviewed changes

ChALkeR reviewed Mar 22, 2018

View reviewed changes

seishun suggested changes Mar 22, 2018

View reviewed changes

ChALkeR reviewed Mar 22, 2018

View reviewed changes

mafintosh reviewed Mar 22, 2018

View reviewed changes

benjamingr reviewed Mar 22, 2018

View reviewed changes

jdalton reviewed Mar 25, 2018

View reviewed changes

targos approved these changes Apr 3, 2018

View reviewed changes

mcollina approved these changes Apr 3, 2018

View reviewed changes

jasnell approved these changes Apr 3, 2018

View reviewed changes

Trott added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 3, 2018

mhdawson approved these changes Apr 3, 2018

View reviewed changes

gibfahn approved these changes Apr 4, 2018

View reviewed changes

cjihrig approved these changes Apr 4, 2018

View reviewed changes

evanlucas approved these changes Apr 4, 2018

View reviewed changes

MylesBorins approved these changes Apr 4, 2018

View reviewed changes

Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 4, 2018

addaleax force-pushed the buffer-node-module branch from 2db2725 to 332271d Compare April 4, 2018 17:44

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018

BridgeAR closed this Apr 9, 2018

BridgeAR mentioned this pull request Apr 10, 2018

buffer: runtime-deprecate Buffer ctor by default #15346

Closed

4 tasks

addaleax mentioned this pull request Apr 14, 2018

coverage.nodejs.org misreporting #19912

Closed

addaleax mentioned this pull request Apr 14, 2018

lib: fix coverage reporting #20035

Closed

2 tasks

seishun mentioned this pull request Jun 15, 2018

buffer: runtime-deprecate Buffer constructor everywhere by default #21351

Closed

4 tasks

ChALkeR mentioned this pull request Jan 31, 2019

[WiP] buffer: runtime-deprecate buffer constructor in sync mode #22584

Closed

4 tasks

buffer: do deprecation warning outside node_modules #19524

buffer: do deprecation warning outside node_modules #19524

Conversation

addaleax commented Mar 22, 2018 • edited Loading

Checklist

addaleax commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun left a comment

Choose a reason for hiding this comment

Trott commented Mar 22, 2018

ChALkeR Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

lpinca commented Mar 22, 2018

mscdex commented Mar 22, 2018

bmeck commented Mar 22, 2018 • edited Loading

MylesBorins commented Mar 22, 2018

mafintosh commented Mar 22, 2018 • edited Loading

mafintosh commented Mar 22, 2018

MylesBorins commented Mar 22, 2018

mafintosh commented Mar 22, 2018

addaleax commented Mar 22, 2018

mafintosh Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Apr 3, 2018

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Apr 3, 2018

BridgeAR commented Apr 3, 2018

seishun commented Apr 3, 2018

mhdawson left a comment

Choose a reason for hiding this comment

addaleax commented Apr 4, 2018

MylesBorins left a comment

Choose a reason for hiding this comment

Trott commented Apr 4, 2018

BridgeAR commented Apr 9, 2018

buffer: do deprecation warning outside `node_modules` #19524

buffer: do deprecation warning outside `node_modules` #19524

addaleax commented Mar 22, 2018 •

edited

Loading

addaleax commented Mar 22, 2018 •

edited

Loading

ChALkeR Mar 22, 2018 •

edited

Loading

ChALkeR Mar 22, 2018 •

edited

Loading

ChALkeR Mar 25, 2018 •

edited

Loading

bmeck commented Mar 22, 2018 •

edited

Loading

mafintosh commented Mar 22, 2018 •

edited

Loading

mafintosh Mar 22, 2018 •

edited

Loading