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

fix(core): prevent hangs due to degenerate lockfile diff #4453

Merged
merged 7 commits into from
May 25, 2022

Conversation

xeger
Copy link
Contributor

@xeger xeger commented May 13, 2022

What's the problem this PR addresses?

This limits the max edit length for jsdiff, keeping it from taking "forever" with very large lockfiles that have substantial changes. It keeps yarn install --immutable from hanging when the prior lockfile was substantially different from the computed lockfile, and thereby fixes #4405.

...

How did you fix it?

The fix relies on a new maxEditLength option for jsdiff, which is available in jsdiff@5.1.0.

If the max edit length is exceeded, jsdiff returns undefined and Project.ts simply skips the verbose diff output.
...

Checklist

  • I have read the Contributing Guide.

  • I have set the packages that need to be released for my changes to be effective.

  • I will check that all automated PR checks pass before the PR gets reviewed.

    • CAVEAT: one failing job that I'm stumped on; marked ready to review to gain advice

@arcanis
Copy link
Member

arcanis commented May 22, 2022

That looks good to me, thanks! Until the package is properly released we can try using it with a git protocol (cc @kpdecker)

@xeger xeger changed the title Xeger/fix/diff hang fix(core): prevent degenerate lockfile diff from hanging the process May 23, 2022
@xeger xeger changed the title fix(core): prevent degenerate lockfile diff from hanging the process fix(core): prevent hangs due to degenerate lockfile diff May 23, 2022
@xeger xeger force-pushed the xeger/fix/diff-hang branch 3 times, most recently from dc412d8 to e669215 Compare May 23, 2022 15:55
@xeger
Copy link
Contributor Author

xeger commented May 23, 2022

FWIW @arcanis I'm not sure git protocol ("diff": "git@github.com:kpdecker/jsdiff.git") will work for this case, since the lib requires transpilation + bundling. I will experiment with using patch:` protocol.

@xeger xeger force-pushed the xeger/fix/diff-hang branch from e669215 to fea0dcb Compare May 23, 2022 16:08
@arcanis
Copy link
Member

arcanis commented May 23, 2022

FWIW @arcanis I'm not sure git protocol ("diff": "git@github.com:kpdecker/jsdiff.git") will work for this case, since the lib requires transpilation + bundling. I will experiment with using patch:` protocol.

I think we can just upgrade to the newly published 5.1!
#4405 (comment)

Although as a trivia, Yarn runs the prepack script when cloning repositories. So even if there's transpilation, if the repository is well configured, it should be possible to use git deps 🙂

@xeger xeger force-pushed the xeger/fix/diff-hang branch from 5458029 to 5af761b Compare May 25, 2022 04:51
@xeger xeger marked this pull request as ready for review May 25, 2022 05:14
@xeger xeger requested a review from arcanis as a code owner May 25, 2022 05:14
@xeger
Copy link
Contributor Author

xeger commented May 25, 2022

I don't know 100% that the build will go green, but the one failing job seemingly has nothing to do with my PR - a typescript package's checksum has begun failing. I experimentally freshened it using the latest from npmjs.org -- hopefully this isn't some obscure supply chain attack!

@merceyz
Copy link
Member

merceyz commented May 25, 2022

hopefully this isn't some obscure supply chain attack!

The only difference between the two is that one is better compressed than the other, otherwise their content is identical. Same thing happens on master though this suddenly changing is worrying.

@arcanis
Copy link
Member

arcanis commented May 25, 2022

The only difference between the two is that one is better compressed than the other, otherwise their content is identical. Same thing happens on master though this suddenly changing is worrying.

Hm perhaps you installed TS 4.7 in a project which had a compression set to 0, which led to the package being added as such into the global cache; then when you upgraded it into the Yarn repo it pulled the 0-compression archive from the global cache and put it in the local one.

However this shouldn't be possible because we append a cache key to the global path, and the compression level is part of it 🤔

@arcanis arcanis merged commit 86cf338 into yarnpkg:master May 25, 2022
@merceyz
Copy link
Member

merceyz commented May 25, 2022

Yeah, that shouldn't be possible, besides the size difference isn't enough for that to be the case.

-11 652 885 bytes
+11 654 629 bytes

@merceyz
Copy link
Member

merceyz commented May 25, 2022

@arcanis It was caused by 2c260ca, it merged in the zip file generated for v3 which doesn't use zlib-ng.

merceyz added a commit that referenced this pull request Jul 20, 2022
* Upgrade diff to 5.0.0

* Add maxEditLength to lockfile patch. Fixes #4405.

* Prepare fix for release

* Upgrade diff to 5.1.0

* Re-resolve typescript 4.7.0-beta from npmjs.org

* chore: versions

Co-authored-by: Tony Spataro <anthony.spataro@appfolio.com>
Co-authored-by: merceyz <merceyz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Yarn 3.2.0 hangs when diffing lockfiles due to endless-loop bug in diff package
3 participants