buffer: do deprecation warning outside node_modules
#19524
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -366,6 +366,61 @@ function spliceOne(list, index) { | |
| list.pop(); | ||
| } | ||
|
|
||
| const kNodeModulesRE = /^(.*)[\\/]node_modules[\\/]/; | ||
|
|
||
| let getStructuredStack; | ||
| let mainPrefix; | ||
|
|
||
| function isInsideNodeModules() { | ||
| // 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. | ||
| // For example, having the main script at `/c/node_modules/d/d.js` | ||
| // would match all code coming from `/c/node_modules/`. | ||
| if (mainPrefix === undefined) { | ||
| const match = process.mainModule && | ||
| process.mainModule.filename && | ||
| process.mainModule.filename.match(kNodeModulesRE); | ||
| mainPrefix = match ? match[0] : ''; | ||
| } | ||
|
|
||
| if (getStructuredStack === undefined) { | ||
| // Lazy-load to avoid a circular dependency. | ||
| 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. | ||
| getStructuredStack = runInNewContext('(' + function() { | ||
| Error.prepareStackTrace = function(err, trace) { | ||
| err.stack = trace; | ||
| }; | ||
| Error.stackTraceLimit = Infinity; | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Ah, disregard this one comment, I think I understand how this works now, sorry =). |
||
|
|
||
| return function structuredStack() { | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| return new Error().stack; | ||
| }; | ||
| } + ')()', {}, { filename: 'structured-stack' }); | ||
| } | ||
|
|
||
| const stack = getStructuredStack(); | ||
|
|
||
| // Iterate over all stack frames and look for the first one not coming | ||
| // from inside Node.js itself: | ||
| 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. | ||
|
There was a problem hiding this comment. Can you explain "it's likely"? There was a problem hiding this comment. @benjamingr You could do the same thing we're doing here, run something inside a If there are more such edge cases, I'm not aware of those, but I'm not comfortable claiming there aren't any either :) |
||
| if (!/^\/|\\/.test(filename)) | ||
| continue; | ||
| const match = filename.match(kNodeModulesRE); | ||
| return !!match && match[0] !== mainPrefix; | ||
| } | ||
|
|
||
| return false; // This should be unreachable. | ||
| } | ||
|
|
||
|
|
||
| module.exports = { | ||
| assertCrypto, | ||
| cachedResult, | ||
|
|
@@ -379,6 +434,7 @@ module.exports = { | |
| getSystemErrorName, | ||
| getIdentificationOf, | ||
| isError, | ||
| isInsideNodeModules, | ||
| join, | ||
|
There was a problem hiding this comment. Just wanted to say that outside the box ideas like |
||
| normalizeEncoding, | ||
| objectToString, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| 'use strict'; | ||
|
|
||
| const child_process = require('child_process'); | ||
| const assert = require('assert'); | ||
| const common = require('../common'); | ||
|
|
||
| if (process.env.NODE_PENDING_DEPRECATION) | ||
| common.skip('test does not work when NODE_PENDING_DEPRECATION is set'); | ||
|
|
||
| function test(main, callSite, expected) { | ||
| const { stderr } = child_process.spawnSync(process.execPath, ['-p', ` | ||
| process.mainModule = { filename: ${JSON.stringify(main)} }; | ||
|
|
||
| vm.runInNewContext('new Buffer(10)', { Buffer }, { | ||
| filename: ${JSON.stringify(callSite)} | ||
| });`], { encoding: 'utf8' }); | ||
| if (expected) | ||
| assert(stderr.includes('[DEP0005] DeprecationWarning'), stderr); | ||
| else | ||
| assert.strictEqual(stderr.trim(), ''); | ||
| } | ||
|
|
||
| test('/a/node_modules/b.js', '/a/node_modules/x.js', true); | ||
| test('/a/node_modules/b.js', '/a/node_modules/foo/node_modules/x.js', false); | ||
| test('/a/node_modules/foo/node_modules/b.js', '/a/node_modules/x.js', false); | ||
| test('/node_modules/foo/b.js', '/node_modules/foo/node_modules/x.js', false); | ||
| test('/a.js', '/b.js', true); | ||
| test('/a.js', '/node_modules/b.js', false); | ||
| test('c:\\a\\node_modules\\b.js', 'c:\\a\\node_modules\\x.js', true); | ||
| test('c:\\a\\node_modules\\b.js', | ||
| 'c:\\a\\node_modules\\foo\\node_modules\\x.js', false); | ||
| test('c:\\node_modules\\foo\\b.js', | ||
| 'c:\\node_modules\\foo\\node_modules\\x.js', false); | ||
| test('c:\\a.js', 'c:\\b.js', true); | ||
| test('c:\\a.js', 'c:\\node_modules\\b.js', false); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // Flags: --no-warnings | ||
| 'use strict'; | ||
|
|
||
| const vm = require('vm'); | ||
| const assert = require('assert'); | ||
| const common = require('../common'); | ||
|
|
||
| if (new Error().stack.includes('node_modules')) | ||
| common.skip('test does not work when inside `node_modules` directory'); | ||
| if (process.env.NODE_PENDING_DEPRECATION) | ||
| common.skip('test does not work when NODE_PENDING_DEPRECATION is set'); | ||
|
|
||
| const bufferWarning = 'Buffer() is deprecated due to security and usability ' + | ||
| 'issues. Please use the Buffer.alloc(), ' + | ||
| 'Buffer.allocUnsafe(), or Buffer.from() methods instead.'; | ||
|
|
||
| process.addListener('warning', common.mustCall((warning) => { | ||
| assert(warning.stack.includes('this_should_emit_a_warning'), warning.stack); | ||
| })); | ||
|
|
||
| vm.runInNewContext('new Buffer(10)', { Buffer }, { | ||
| filename: '/a/node_modules/b' | ||
| }); | ||
|
|
||
| common.expectWarning('DeprecationWarning', bufferWarning, 'DEP0005'); | ||
|
|
||
| vm.runInNewContext('new Buffer(10)', { Buffer }, { | ||
| filename: '/this_should_emit_a_warning' | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still suggest bailing out after 10k attempts (which are about ~0.1 second).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.