node icon indicating copy to clipboard operation
node copied to clipboard

util: fix crashing when emitting new Buffer() deprecation warning

Open Uzlopak opened this issue 1 year ago • 8 comments

Closes #53075

Uzlopak avatar May 21 '24 21:05 Uzlopak

@anonrig

I tried to implement a test few weeks ago, and now again invested an hour. I cant catch the thrown error.

Just merge it please.

Uzlopak avatar Jun 27 '24 10:06 Uzlopak

Well, tbh a comment is superficial. getFileName can return undefined

Do you insist on the comment?

Uzlopak avatar Jun 27 '24 13:06 Uzlopak

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 :)

naugtur avatar Jun 27 '24 21:06 naugtur

Well, just for the context:

This is a potential test: try { new Buffer(1); console.log(0); ; process.exit(0); } catch (e) { console.log(1); process.exit(1); } I just get the error, if I run node as repl. So run node and copy paste it. Enter. And you get a exit code of 1

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?

Uzlopak avatar Jun 27 '24 21:06 Uzlopak

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.

naugtur avatar Jun 27 '24 21:06 naugtur

CI: https://ci.nodejs.org/job/node-test-pull-request/60001/

nodejs-github-bot avatar Jun 28 '24 00:06 nodejs-github-bot

what now?

@anonrig

Uzlopak avatar Jun 28 '24 21:06 Uzlopak

what now?

@anonrig

There is a CI lockdown. We need to wait couple of days to land this.

anonrig avatar Jun 28 '24 21:06 anonrig

@anonrig Can we merge it now?

Uzlopak avatar Jul 03 '24 10:07 Uzlopak

@anonrig

Can we now merge it?

Uzlopak avatar Jul 09 '24 20:07 Uzlopak

@jasnell @anonrig @mcollina

is it mergable?

Uzlopak avatar Jul 11 '24 08:07 Uzlopak

CI: https://ci.nodejs.org/job/node-test-pull-request/60239/

nodejs-github-bot avatar Jul 11 '24 09:07 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/60240/

nodejs-github-bot avatar Jul 11 '24 09:07 nodejs-github-bot

Landed in 096e44a8a9dd20e3159ee15bc2563c6a58dc344f

nodejs-github-bot avatar Jul 11 '24 12:07 nodejs-github-bot

Is there an ETA for this getting backported to v20?

It seems that it's not part of just released v20.16.0? (#53945)

legobeat avatar Jul 26 '24 04:07 legobeat

Is there an ETA for this getting backported to v20?

It seems that it's not part of just released v20.16.0? (#53945)

I've added the LTS watch label for v20.

If a someone disagrees with the addition of that label, feel free to remove it.

avivkeller avatar Aug 11 '24 01:08 avivkeller