Core: Fix startup hang caused by watchStorySpecifiers

[heyimalex]

Closes #18881

What I did

Changed how watchpack gets initialized in watchStorySpecifiers. Previously we:

• Passed in the directories from the story specifiers directly into watchpack as relative paths.

Now we:

• Pass in the directories from the story specifiers and all nested files and directories.
• Pass everything in as absolute paths instead of relative paths.

This fixes two issues:

1. watchpack blocks for a long time closing FSWatchers on osx webpack/watchpack#222: Because we passed in only the top level directories, watchpack's recursive watching triggered a lot of churn once you cross the WATCHPACK_WATCHER_LIMIT threshold. The outcome was multi-second hangs on startup. See the repro in the linked issue, it's incredibly simple and reliably fails on OSX.
2. Twice as many FSWatchers created when using storyStoreV7 #18881: Because we're now passing the same absolute paths to watchpack that webpack passes internally, it's able to internally re-use FSWatcher objects (given the same version of watchpack is being used under the hood).

Reproduce the original issue by checking out next and setting up a sandbox. cd into the sandbox.

Run yarn storybook and note the time to startup. On my machine, 334 ms for manager and 880 ms for preview.

Open node and run this to create a bunch of empty subdirectories within the sandbox.

const fs = require("fs");
const path = require("path");

const base = path.resolve("./src/extra");

// Generate a bunch of directories to simulate watchpack runs against.
function makeDirectoryTree(depth, dirsPerLevel) {
  fs.mkdirSync(base);
  let total = 0;
  let level = [base];
  for (let i = 0; i < depth; i++) {
    const next = [];
    for (const dir of level) {
      for (let j = 0; j < dirsPerLevel; j++) {
        const subdir = path.join(dir, `subdir-${i}-${j}`);
        fs.mkdirSync(subdir);
        total += 1;
        next.push(subdir);
      }
    }
    level = next;
  }
  console.log(`${total} directories created`);
}

makeDirectoryTree(5, 6);

Now run yarn storybook again. On my machine, 44 s for manager and 703 ms for preview.

Checkout this branch and run the test again. On my machine, 254 ms for manager and 669 ms for preview.

Checklist for Contributors

Testing

Updated some tests, didn't love how it went.

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

Did console.log testing / manual testing on the shape of everything in a sandbox.

Documentation

N/A

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

[chrisgarber]

Big savings 👍 🎉

[yannbf]

Hey @heyimalex thanks a lot for your PR. So the storyStoreV7 flag doesn't exist anymore in Storybook 8. I'm assuming you are still facing the same issues in Storybook 8?

@tmeasday could you please take a look at this PR? Thanks!

[shilman]

@heyimalex thanks so much for putting this together. i just created a new PR that adds another Watchpack watcher. It's watching the config directory (e.g. .storybook/) and has slightly different logic than the current set of watchers. i don't know much about the performance implications of my work, but if you could possibly take a look and let me know if you have any concerns, i would really appreciate that!

[heyimalex]

@yannbf Sorry, I've updated the title. Yes, this is still an issue, it's been around for as long as watchStorySpecifiers has existed, but just now getting around to upstreaming it. We've had a similar patch in place at dayjob for almost two years now, back when storyStoreV7 was experimental 😅

@shilman I believe the same issue will exist, but it's IMO less likely to be problematic because .storybook doesn't tend to contain that many files. Default watcher limit on osx is ~2k, but that applies to all nested directories beneath the passed directory (if they don't match the ignore rules), so most folks will only see this issue when the size of their src / story containing dir becomes fairly large / deeply nested.

[tmeasday]

This seems great @heyimalex. Some questions:

Pass in the directories from the story specifiers and all nested files and directories.

I'm not really understanding why given we pass in the same set of directories it doesn't actually end up needing to recurse anyway, but it seems like you have a good handle on this :)

given the same version of watchpack is being used under the hood

I suppose there is no real way we can guarantee this, is there? But it is good that the perf boost is there in the case that it does happen.

[tmeasday]

Code changes LGTM

[tmeasday]

Does this need to be sync? If we do it async might it give other parts of our boot up a chance to do work while waiting on FS?

[heyimalex]

I don't think it must be sync, we'd just create the watcher asynchronously after directory walking completes. IMO performance implications here are not significant tho, even with 10k directories readdirSync calls seem to complete in negligible time.

[heyimalex]

I'm not really understanding why given we pass in the same set of directories it doesn't actually end up needing to recurse anyway, but it seems like you have a good handle on this :)

Forgive me if this isn't perfectly accurate, haven't really worked on this in a couple of years.

So watchpack has direct watchers and recursive watchers. It has a function called reducePlan which takes a mapping of paths to watch, and intelligently collapses them down, creating recursive watchers to replace multiple direct watchers, so that the total number of watchers is below the limit.

When passing in all directories, this reduce plan runs once, and (given the repro conditions) watchpack will recognize that a single recursive watcher is the best move.

When passing in a single top level directory, essentially what happens is every time a readdir call completes, watchpack calls watch on each of the returned directories, and reducePlan runs again. You can see a "trace" of this behavior (created with hacky console log instrumenting) here. It ultimately gets to the same plan (see the final RecursiveWatcher.ctor: /Users/alex/projects/watchpack/fake-dir-base), but the process churns through the creation and close of a bunch of individual watchers, as reducePlan is working on an incomplete picture.

I suppose there is no real way we can guarantee this, is there? But it is good that the perf boost is there in the case that it does happen.

No, there isn't, this is more of a best-case-scenario optimization.

Also, just want to call out another possible solution: using a different library. The actual thing we're using watchpack for seems pretty simple on its face: we need to watch directories and trigger a callback when files that match some pattern nested within those directories are added, removed, or updated. Watchpack was probably seen as an easy enough way to get there, and something we already had a transitive dependency on, but given the level of footgunnery here maybe a different dep is appropriate.

[tmeasday]

Watchpack was probably seen as an easy enough way to get there, and something we already had a transitive dependency on, but given the level of footgunnery here maybe a different dep is appropriate.

Agree 💯 ! If you have another suggestion I'm all ears!

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 80f8dc0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx run-many -t build -c production --parallel=3

---

Sent with 💌 from NxCloud.

[heyimalex]

After some light research, evaluating file watching libs is really hard hahaha. It seems like @parcel/watcher may be a good choice, as it's what's currently used by vscode. chokidar is widely deployed, but vscode moved away from it. Many other small libraries that seem to have spotty cross platform support or not be maintained.

[heyimalex]

Alternative branch here, but this PR is definitely lower risk.

[tmeasday]

Thanks @heyimalex. We are in feature freeze right now but I'll aim to get this or the other one merged soon after 8.1 goes out.

[shilman]

@heyimalex thanks for your patience on this. we're releasing 8.1 in a day or two and i'm planning to merge this as soon as we get that out so it has some time to bake for 8.2. excellent work!!! 🙌

[tmeasday]

@shilman we should consider the other PR that migrates us to Parcel's file watcher.

[shilman]

Greate call @tmeasday -- I totally missed that. Let's discuss at the next available opportunity!