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

[ChALkeR]

This is a work in progress, temporarily alternative for #21351 for now.
Judging by the current situation, I think that #21351 would almost certainly not land in v11.0, but I hope this approach can (if ecosystem tests show that is ok, see below).

The idea is to reduce the number of warnings users see and make them reproducable, while still printing warnings for code inside of node_modules, long-term plan being to land #21351 (or alternative) once it becomes viable in a sense that benefits outweight potential user disturbance (see refs below).

The reasoning why we need this is that the current warnings for code outside of node_modules do not help much in reducing the usage of unsafe buffer constructor — they do help by downloads/month metrics, but it that did not stop the number of modules using unsafe Buffer API from growing.
See #21351 (comment) and #21351 (comment) for more info and reasoning.

This has to go through CITGM to check how this affects popular modules.

This is currently more like a proof-of-concept to test things around and not near being completed, so what we need now is to understand how would this affect user modules, and if we want something like this in 11.0. For that, I think that this should be enough and such things could be already tested given this code.

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

/cc @nodejs/tsc, @targos @seishun @fhinkel @danbev @addaleax

[nodejs-github-bot]

@ChALkeR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/723/pipeline

[ChALkeR]

This shouldn't expose undocumented things directly on process — that will change.

[targos]

I'm +1 on trying this approach

[apapirovski]

I'm +1 to this approach. (Just please don't spell out synchronous lol... sync is good enough).

[mcollina]

I am -1 to this approach unless it’s proven that the vast majority of affected modules are not showing the warning. I’m referring to old modules that have not been updated in ages.

cc @mafintosh

[seishun]

I am -1 to this approach unless it’s proven that the vast majority of affected modules are not showing the warning.

If the vast majority of affected modules are not showing the warning, then the warning is useless.

[ChALkeR]

@mcollina @seishun

vast majority of affected modules

I don't think that's even a correct question, to speak about affected modules.

Modules provide APIs. Those could be sync or async, but most things happen directly on function calls (with some noticable exceptions, yes).
What matters is when those functions are called from users code and how the module is used.

Assuming that most of the code in real-world apps is async, I expect that this will hit much less applications than a full runtime deprecation would.

This also will make sure that the warning doesn't unexpectedly show up during an application lifetime while it is running — only at start-up where in would most likely be noticed by the developer immediately.

At the same time, that should affect some apps, and I anticipate that being enough to make the affected modules either drop their usage of unsafe Buffer constructor (hopefully not only in the affected sync codepath, but in all code), or migrate to other non-affected modules in the dependency chain in cases where unsupported modules are used.

CITGM tests are of course needed though — knowing how much testcases would be hit by this should give hints if this really helps reducing the disturbance compared to a full runtime deprecation.

I would appreciate any help with running this through CITGM btw.

[BridgeAR]

+1 on the approach

[targos]

I don't know if it's possible with the CI job, but when this is ready I'll try to run CITGM with NODE_OPTIONS="--throw-deprecation".

[mafintosh]

Does anyone have a real-world example of a synchronous/first-tick new Buffer usage that is actually unsafe? As I see this now, this might just create more noise without finding any modules where moving to Buffer.from/alloc would help in practice.

@ChALkeR anyway you can share the updated ecosystem data with me? i'm curious to see what kind of newly published modules are using new Buffer.

[ChALkeR]

@mafintosh Done. You should have received an invitation.

[addaleax]

I’m sorry, I can’t quite get behind this either and am -1 as well.

This will most likely not help with problematic usage, and I think it’s too early to judge the effects of our changes to Node 10 (warning outside node_modules) completely before it has been the main production environment.

That is, I do not think we should take any action on this topic in Node 11, or possibly ever.

[seishun]

Adding to the agenda because a decision needs to be made soon, otherwise we'll just run out of time.

[mcollina]

@seishun what do you think of the analysis in #21351 (comment)? It seems our current strategy is indeed working, and this is not needed after all.

[seishun]

It seems to be working for new modules, but the purpose of this PR (and #21351) is to decrease the usage in existing modules.

[targos]

I didn't try with this change yet, but at https://github.com/nodejs/citgm/projects/2#column-3451048 you can see the list of modules tested by citgm whose test suite already emits a deprecation warning with node master.

[ChALkeR]

@targos, that doesn't affect real-life setups that are consuming those libs, because those end up in node_modules.

This change will affect some consumers of the libs though, so what makes the difference here – how many setups would be affected? Hopefully not too much, but not zero - so that those issues would get noticed.

Perhaps for the citgm testing of this proposal it would make sense to comment out / disable the check that always displays the warning outside of node_modules dir – that isn't relevant here and would not affect users, what is relevant is the warning that gets displayed even inside of node_modules.

[ofrobots]

I'm -1 on this at this point for Node 11. I do not have a good sense of the concrete benefit and whether they out weight the negatives.

[jasnell]

Definite -1 for 11

[gibfahn]

-1 for 11

[Trott]

With @gibfahn's vote, that gets us to resolution as far as 11.x goes: This is not going in the 11.x release. It may still end up in the 12.x release.

@seishun Is it OK to pull this from the TSC agenda for this week or is there still something you'd want the TSC to address this week?

[thefourtheye]

Abstaining.

[seishun]

@Trott fair enough, it seems there is nothing else to discuss.

[ChALkeR]

Ok, let's wait to see the usage evolves.
I would prefer to keep this open in the meantime.

[Trott]

Is it time to put this on the TSC agenda to discuss whether it would be permissible to land in 12.x or not?

[Trott]

I imagine we need to update the comment below to reflect this.

[ChALkeR]

@Trott I will try to take another look and see what changed over these months in a week or two. Let's delay until then.

[rvagg]

@ChALkeR we discussed this briefly in the TSC meeting today, along with #21351 and agreed that it'd be good to see up to date usage data. Did you get gzmnid set up and working on that new server, are you able to run another analysis easily?

[ChALkeR]

@rvagg Yes, I migrated everything to the new server, thanks!

The last datapoint in those stats was for 2018-07-24, I will add two new datapoints: for 2018-11-12 and for 2019-01-28 (based on datasets created on the new server). I will try to do that later today or tomorrow, I guess.

[mcollina]

I have one more datapoint. Applications that use Buffer() in a hot path as a performance hit as isInsideNodeModules is expensive to compute.

I’ve been actively seen those in real-world flamegraph since v10 became LTS. I think the slow-down strategy is somehow already been implemented.

[ChALkeR]

@mcollina this approach is not expected to add any visible additional slowdown.

Slowdown being caused by #19524 was expected (and partially intentional?), and it is sure expected to appear on flamegraphs, but it is constrained, so it shouldn't cause issues in long-running apps like servers. See #19524 (comment), for example.

[ChALkeR]

@rvagg Ok, I have new data.

In short: total affected packages count slowly grows, downloads/count normalized on today stats continues to drop (popular packages migrate off), but the actual downloads/month still seem to grow because of the fast overall ecosystem growth.

I will add some more metrics (based on stats on different dates) in the next few days, also I'll look deeper inside the data (i.e. which are the top packages, are those false positives or not, how easy it is to fix them).

[Fishrock123]

Can I add to the record that I am permanently abstaining from any decision regarding the Buffer constructor?

[BridgeAR]

What should be done with this? Should this come up again for Node.js 12?

[ChALkeR]

@BridgeAR We could re-evaluate this in time for 13, based on the ecosystem state.

But I would go with #28439 first (which is a semver-minor).

[ChALkeR]

Let's close this for now, might want to revisit later.
I would prefer approach from #28439 for the time being.