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.

Sign up for GitHub

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

Jump to bottom

Revert "fs: add v8 fast api to closeSync" #53904

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Revert "fs: add v8 fast api to closeSync" #53904

merged 1 commit into from
Jul 18, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Jul 17, 2024 •
edited
Loading

This reverts commit ed6f45b (#53627). That commit appears to have introduced regressions into 22.5.0, so this PR reverts it.

Fixes #53902
Ref: yarnpkg/berry#6398
Ref: npm/cli#7657
(And all linked issues in the ones above)

CC @anonrig

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 17, 2024
@RedYetiDev RedYetiDev added revert PRs that revert previously landed PRs. fs Issues and PRs related to the fs subsystem / file system. labels Jul 17, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review July 17, 2024 18:49
@RedYetiDev RedYetiDev mentioned this pull request Jul 17, 2024
Closed
@anonrig
Copy link
Member

anonrig commented Jul 17, 2024 •
edited
Loading

Let's wait for landing this PR for a couple days in case we can find a solution that doesn't require a revert, unless we ship a new version soon.

@RedYetiDev

This comment was marked as outdated.

Sign in to view
@RedYetiDev RedYetiDev closed this Jul 17, 2024
@RedYetiDev RedYetiDev reopened this Jul 17, 2024
@RedYetiDev
Copy link
Member Author

Re-opening per #53902 (comment)

@RedYetiDev
Copy link
Member Author

Let's wait for landing this PR for a couple days in case we can find a solution that doesn't require a revert, unless we ship a new version soon.

@anonrig (I know it's only been a few hours) The other PR (according to #53902 (comment)) only resolves part of the issue, and reverting it will resolve the other part. IMO the revert should be sent out in a patch for 22.5.X, so that users have a stable 22.5?

@anonrig
Copy link
Member

anonrig commented Jul 17, 2024

@anonrig (I know it's only been a few hours) The other PR (according to #53902 (comment)) only resolves part of the issue, and reverting it will resolve the other part. IMO the revert should be sent out in a patch for 22.5.X, so that users have a stable 22.5?

What other issue are you referring to? #53910 fixes the crash.

@RedYetiDev
Copy link
Member Author

According to #53902 (comment), Yarn scripts fail with:

Usage Error: Couldn't find the node_modules state file - running an install might help (findPackageLocation)

I've confirmed this is the case with 22.5.0, but I haven't tested your patch.

@RedYetiDev
Copy link
Member Author

I'm cloning your PR now, so I'll test whether or not it fixes the yarn issue

@merceyz
Copy link
Member

merceyz commented Jul 17, 2024

According to #53902 (comment), Yarn scripts fail with:

Usage Error: Couldn't find the node_modules state file - running an install might help (findPackageLocation)

I've confirmed this is the case with 22.5.0, but I haven't tested your patch.

Quoting myself from #53902 (comment):

Note that the issue happens in the yarn step where the process exits early without an error, yarn build fails because yarn failed.

So that issue isn't reproduced by yarn build but in yarn.

@RedYetiDev
Copy link
Member Author

Does @anonrig's PR fix the issue?

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 18, 2024 •
edited
Loading

After testing:

  • Current (v22.5.0): yarn install -> Hangs on 'Link Step'
  • @anonrig's PR: yarn install -> Hangs on 'Link Step'
  • My PR: yarn install -> Success

@anonrig
Copy link
Member

anonrig commented Jul 18, 2024

After testing:

  • Current (v22.5.0): yarn install -> Hangs on 'Link Step'
  • @anonrig's PR: yarn install -> Hangs on 'Link Step'
  • My PR: yarn install -> Success

@RedYetiDev Reverting this PR will fix this problem but postpone the timeline to find the root cause of the underlying problem. Since V8 fast APIs are added constantly, and will be added in the future, it is important to gather as much as evidence as we can in order to increase our test suite to prevent regressions. Since, we have time, we should merge this PR as a last resort in case we can't find a solution before we create a new Node.js release. Right now, we have time, and without finding the root cause, merging this PR will not help the project.

I understand your eagerness but we need to learn from our bugs/mistakes and missing test coverage so that we don't break Yarn in the future specific to this.

As of now, I recommend gathering as much as data we can, to find a suitable regression, that doesn't force us to run an external script.

@RedYetiDev
Copy link
Member Author

Right now, we have time, and without finding the root cause, merging this PR will not help the project.

Got it. Thanks, I exaggerated, and you are right. We do have time to find the cause. I'll look into it more rather than to just revert.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 18, 2024 •
edited by richardlau
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/60422/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3460/

@richardlau
Copy link
Member

I think we want to get a fixed Node.js 22 release out for #53902 sooner rather than later.

If #53910 is ready (looks like it might have related CI failures) then I'll have no objections to landing that instead of this revert.

@merceyz
Copy link
Member

merceyz commented Jul 18, 2024 •
edited
Loading

Does @anonrig's PR fix the issue?

Not the issue affecting Yarn, no.

@RedYetiDev RedYetiDev mentioned this pull request Jul 18, 2024
Closed
2 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 18, 2024 •
edited
Loading

I think we want to get a fixed Node.js 22 release out for #53902 sooner rather than later.

Given the widespread issues, I agree.

CI has passed for this PR.

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2024
@anonrig
Copy link
Member

anonrig commented Jul 18, 2024

cc @nodejs/releasers can we release a new version with this commit?

@anonrig anonrig added fast-track PRs that do not need to wait for 48 hours to land. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 18, 2024
@github-actions GitHub Actions
Copy link
Contributor

Fast-track has been requested by @anonrig. Please 👍 to approve.

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2024
@nodejs-github-bot nodejs-github-bot merged commit c0c7e86 into nodejs:main Jul 18, 2024
117 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c0c7e86

@RedYetiDev RedYetiDev mentioned this pull request Jul 19, 2024
Open
@RedYetiDev
Copy link
Member Author

cc @nodejs/releasers can we release a new version with this commit?

Agreed. as discussed here, other issues, and slack, I believe the best course of action is to release a patch, and urge users to update.

richardlau pushed a commit that referenced this pull request Jul 19, 2024
@RedYetiDev @richardlau
Revert "fs: add v8 fast api to closeSync"
e2deeed 
This reverts commit ed6f45b.

PR-URL: #53904
Refs: yarnpkg/berry#6398
Refs: npm/cli#7657
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@richardlau richardlau mentioned this pull request Jul 19, 2024
Merged
@cyyynthia cyyynthia mentioned this pull request Jul 19, 2024
Closed
@richardlau richardlau added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. revert PRs that revert previously landed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node 22.5.0 started to crash and hangs on different cases
7 participants
@RedYetiDev @anonrig @merceyz @nodejs-github-bot @richardlau @Trott @lpinca