-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
util: fix crashing when emitting new Buffer() deprecation warning #53089
Conversation
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.
Can you add a test?
I tried to implement a test few weeks ago, and now again invested an hour. Just merge it please. |
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 suggest leaving a line of comment explaining it's meant to recognize that we're dealing with a Node.js core module.
Well, tbh a comment is superficial. Do you insist on the comment? |
I don't insist. I found the deleted comment useful to understand the PR without reading more context. By adding an approval with it I intended to say it's good as is. Also, thanks for the fix. I work with some people who are waiting for it to ship :) |
Well, just for the context: This is a potential test: store the line in a file, e.g. test.js, and run it: node test.js It returns 0 This is because if you store it in a file, you have a filename So this was my approach to write a test, but it doesnt work. 'use strict';
require('../common');
const assert = require('assert');
const { spawn } = require('child_process');
const child = spawn(process.execPath, [
'--interactive',
]);
child.stdin.end(`try { new Buffer(1) } catch (e) {process.exit(1)}`);
child.on('exit', (code) => {
assert.strictEqual(code, 1);
}); This is what I get: aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ node ./test/parallel/test-buffer-constructor-no-throw.js
node:assert:126
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
0 !== 1
at ChildProcess.<anonymous> (/home/aras/workspace/node/test/parallel/test-buffer-constructor-no-throw.js:13:10)
at ChildProcess.emit (node:events:520:28)
at ChildProcess._handle.onexit (node:internal/child_process:294:12) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 0,
expected: 1,
operator: 'strictEqual'
}
Node.js v23.0.0-nightly20240507be8d64ec14 I have no clue, why the test isnt passing. Maybe somebody has some deeper insight. Maybe I just have to spawn the node process differently? |
Tomorrow is my last day before vacation but I'll see if I can put some time in an attempt to isolate the code that throws this error for us. |
what now? |
There is a CI lockdown. We need to wait couple of days to land this. |
@anonrig |
Can we now merge it? |
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.
lgtm
Landed in 096e44a |
PR-URL: #53089 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #53089 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Closes #53075