Skip to content
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

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented May 21, 2024

Closes #53075

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels May 21, 2024
@Uzlopak Uzlopak changed the title util: fix WASM crashes on Node v22.2.0 if it reaches the new Buffer() deprecation warning May 21, 2024
Copy link
Member

@anonrig anonrig left a 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?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 27, 2024

@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.

Copy link

@naugtur naugtur left a 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.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 27, 2024

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

Do you insist on the comment?

@naugtur
Copy link

naugtur commented Jun 27, 2024

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

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 27, 2024

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?

@naugtur
Copy link

naugtur commented Jun 27, 2024

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.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2024
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 28, 2024

what now?

@anonrig

@anonrig
Copy link
Member

anonrig commented Jun 28, 2024

what now?

@anonrig

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

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 3, 2024

@anonrig
Can we merge it now?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 9, 2024

@anonrig

Can we now merge it?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 11, 2024

@jasnell
@anonrig
@mcollina

is it mergable?

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2024
@nodejs-github-bot nodejs-github-bot merged commit 096e44a into nodejs:main Jul 11, 2024
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 096e44a

@Uzlopak Uzlopak deleted the fix-53075 branch July 11, 2024 12:14
aduh95 pushed a commit that referenced this pull request Jul 12, 2024


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>
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
aduh95 pushed a commit that referenced this pull request Jul 16, 2024


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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
7 participants