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

[xeger]

Self-service

• I'd be willing to implement a fix

Describe the bug

When I yarn install --immutable inside a container as part of my CI process, Yarn detects that the lockfile would be updated and attempts to show me the diff before exiting with an error.

For the particular lockfile that I am using with my project, the diff never completes: yarn runs forever, using 100% CPU, until I kill it.

To reproduce

I have isolated the problem to the diff package, a dependency of Yarn, and filed an issue with the maintainer of that package, including a self-contained reproduction that does not depend on yarn.

Environment

System:
    OS: Linux 5.10 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    CPU: (4) x64 unknown
  Binaries:
    Node: 16.14.2 - /tmp/xfs-5c2a3254/node
    Yarn: 4.0.0-rc.3.git.20220426.hash-67ce47fc6 - /tmp/xfs-5c2a3254/yarn
    npm: 8.5.0 - /usr/local/bin/npm

Additional context

I am at an impasse: I can't upgrade my project to Yarn 2/3 as long as those tools are unstable and prone to disrupt my development workflow. It seems like Yarn's attempt to provide a UI nicety (colorized diffs) is making it fundamentally unstable.

The hang happens both with diff@4.0.1 (used by yarn@3.2.0) and the most recent build, diff@5.0.0 -- so a simple upgrade of diff will not suffice to fix the issue.

Short of fixing the underlying bug, I'm not sure how the Yarn team could proceed. Personally, I don't need the diff in the environments where this hang occurs -- or ever. A .yarnrc.yml flag to disable diffs would be an adequate workaround for me.

I'm happy to attempt whatever feature the team decides is appropriate. Personally, I'm not sure I trust the diff package enough at this point to depend on it for a tool that is a core part of my software development lifecycle. I appreciate that yarn's goal is to run with no dependencies on the OS, however, and it seems that Yarn patches also use diff -- so perhaps the Yarn team can provide some help to the maintainer of diff.

[cuyl]

I've had this problem before. see #3764

[arcanis]

Yes, this one is annoying - I think the best would be to switch to using git diff. We already do so in other places (yarn patch-commit), and given that printing the diff is a non-essential feature we could just skip it if git diff crashes (for example because git is missing). Would you be open to try out implementing the fix?

Note that if we do this, another improvement I'd like to see would be to remove the diff printing if it's larger than, say, 200 lines (which is what I suspect diff would return ... if it returned 😅). This thing is mostly intended to quickly spot the one dependency being missing, not see a huge diff caused by lockfile regenerations or conflict merges.

[xeger]

I'm willing to try porting this fuctionality to diff or git diff -- I did some work on it earlier this morning and it seems straightforward to call git diff, and less straightforward but still feasible to parse the output into a structured diff so we can preserve syntax highlighting. (Actually, plain old diff might be a better fit for parsing the output...)

I wanted to offer up a potentially different fix, which is to add an early-termination feature to jsdiff, as seen here:
kpdecker/jsdiff#365

If @kpdecker agrees with this change -- and I imagine that the rationale that "yarn is broken" might get his attention! -- then we can minimize change and limit our execution time with a reasonable edit distance of ~100 lines, which is the most anyone ought to want to see on screen.

If I can get in touch with him, I'll see about getting that fix merged - and might need to bump yarn's diff to 5.0, or might just backport it to 4.x depending on the maintainer's whims.

Thoughts on which approach is best for yarn? If we'd rather move toward eliminating a dependency, I'm happy to switch to git diff or diff.

[rachelslurs]

Hat tip to @apenney for this find:

As a short term fix, you can increase the --max-old-space-size in your NODE_OPTIONS.

So say you're running this in docker, you'd add to your Dockerfile something like ENV NODE_OPTIONS=--max-old-space-size=8192

[xeger]

It seems like @kpdecker is dormant, so, I propose we forge ahead with a solution that uses git diff (and emulates the behavior of patch-commit w/r/t invoking Git, etc). @arcanis did you imagine we would retain the color syntax highlighting, or just notify with the raw output of git diff?

[kpdecker]

Glad to add a maintainer to diff project if maintainers of yarn vouch for them.

[kpdecker]

More specifically a collaborator on the yarn npm package vouches for that individual. I don't have the time to maintain the package, but want to be careful with who has publish access given how many machines this code ends up on.

[xeger]

Understood. I'm a yarn neophyte, and so probably not vouchable-for, but perhaps one of the core collaborators will take you up on the offer. Regardless, I will contribute a yarn PR to use jsdiff 5 once the new build is published.

[kpdecker]

Merged PR. Will have to dust off the build toolchain to get a release out. Will try to tackle this weekend.

[kpdecker]

5.1.0 just published.