NormalModule.needRebuild cache calculations seem to be wrong for filesystems that don't support millisecond precision mtimes

[timmfin]

I was writing some integrations tests for broccoli-webpack-cached (trying to test that broccoli and webpack caching were working well together) and I was getting pretty unexpected, seemingly non-deterministic behavior. That lead me to digging into how Webpack caches things, namely how:

• NormalModule saves a buildTimestamp every time a file is built
• At the start of a new compile, Webpack gathers the mtimes from the filesystem for of every file in the dependency tree via CachePlugin
• And then during the next compile, compares the new mtime against the previously saved buildTimestamp for each file in needsRebuilt

That all makes sense, but the problem I see is that buildTimestamp is saved with millisecond-level precision. However, many filesystems out there (in my case, HFS on OSX) only support second-level precision. So this can happen:

• A compile starts, let's say at at 13:50:00 and 100ms (hour, min, second, and ms). That is saved to this.buildTimestamp.
• The compile ends, the file is modified immediately at 13:50:00 and 500ms. But the timestamp saved in the compiler.fileTimestamps object has no millisecond precision, so is only 13:50:00 and 0ms
• Another compile is kicked off soon after and checks if that file needs to be rebuilt. It compares to see if the new filesystem timestamp (13:50:00 and 0ms) is >= the previous buildTimestamp (13:50:00 and 500ms) and decides there has not been a file modification. Even though there definitely was a change to the file, it just happened within the same second and was ignored.

I verified this problem goes away when I force NormalModule to truncate milliseconds from saved the buildTimestamp, like so:

this.buildTimestamp = Math.floor(new Date().getTime() / 1000) * 1000;

But that likely isn't a good change. Ideally, I think that truncation would only happen if we can detect that the input filesystem only has second-level precision (but I didn't look deep how hard/easy that is).

I realize that I am much more likely to run into this issue when writing test code, and that is pretty different than a typical local dev server/watch/build script use-case. But I gotta think that this bug even rears its head in normal scenarios from time to time... well that is unless I've missed something.

ps: Above Webpack github links are to 1.x not 2.x-beta source.

pps: Node didn't always, but now does support millisecond precision for filesystems that support it.

[sokra]

This is pretty critical...

[sokra]

okey, I tried to fix it... but unsure if it's working correctly. Could you run your test agains the webpack master branch with updated watchpack dependency.

[timmfin]

Awesome, thanks.

To test I first backported your fix on top of v1.12.13, since there were several other things I'd need to change to get on webpack 2.x.

After that, things were almost working, except for a couple small things. I needed this this fix to ensure the timestamps were actual timestamps and summed into ints rather than a concatenated string.

And also, I needed to make sure that the timestamp adjustment happened after the async.forEach loop was finished.

After that tiny fix, things are golden 😸. Though I thought I could make the FS_ACCURENCY heuristic a bit better:

Tweak calculation of FS_ACCURENCY

Before this accuracy detection code would end up leaving FS_ACCURENCY equal to 10000 or 2000 on my machine (depending on whether the time stamp was something like 1455197568000 or 1455197569000). Which was pretty far away from the actual precision of one second… and concerning to me if that would make us cache too few things.

So my attempt here was to assume that FS accuracy was always an order of magnitude—1, 10, 100, or 1000 milliseconds—and be a little more aggressive to set the FS_ACCURENCY lower. For example, in the new code, any time a timestamp is seen with 3 trailing zeros (mtime % 10000), I assume that the precision is 1000ms.

Those changes look good to you? Let me know if you'd like to me make an actual PR.

[timmfin]

@sokra, I continued to run into more issues†, so I tried a different approach. Instead of accounting for filesystem accuracy, I tried to instead improve webpack to only refer to and compare actual file mtimes and content hashes‡. I spiked on that, with a couple tests to confirm, over in my fork's mtime-for-caching branch: v1.12.13...timmfin:mtime-for-caching (note it is on Webpack 1.x and also requires these webpack-core changes.

Additionally, to more concretely illustrate this issue (and another I found with CachePlugin), I created a webpack-cache-issues example repo with scripts to replicate the cache issues.

Note, my proposed code changes are far from perfect. Not only do I not have a full understanding of the whole Webpack code base (for example, are NormalModules the only thing I need to worry about?), I also ended up needing to make the needRebuild function async and refactor some core methods. And I'm reading the file from the inputFileSystem in two separate spots. So consider my code a prototype, but not recommendation of the exact implementation. Fortunately, I do have four new tests to test these issues and I've made sure my prototype passes the rest of the test suite. Plus, I'd be very glad to work on this more given some pointers and recommendations.

†: The other issue I was running into was that Webpack's previous caching logic (in particular, the logic when directly using the Webpack node API) assumed that the there was little-to-no time elapsed between when he original file modification occurs and when Webpack starts to build the file. Since the buildTimestamp is merely a new Date it can be problematic to compare it to the file's mtime. Because if Webpack doesn't actually start to build (and save a new buildTimestamp) immediately after the file is modified, another modification could sneak in and never get built.

Here's one (but not the only) example of how this goes wrong, even if the filesystem has millisecond-level precision:

1. I run broccoli server to start my build (let's say it started at 12:00pm and 0 seconds)
2. Various things happen, maybe taking a few seconds to do other broccoli build tasks.
3. (Sometime during this build, I modify X.js again—new mtime of 12:00pm and 2 seconds—but since the broccoli server/watcher is blocking, that change only queues up another build for when the current one finishes.)
4. Then my Brocfile.js delegates to the Webpack compiler (via broccoli-webpack-cached)
5. A new buildTimestamp is created for X.js (let's say at 12:00pm and 5 seconds) and Webpack builds against the original content (aka, those modifications made at 12:00pm and 2 seconds were not passed on by broccoli yet)
6. The Webpack compile finishes, and then the broccoli build finishes.
7. Broccoli kicks off the queued rebuild from X.js's earlier change.
8. Broccoli again does other build steps, which may take a few seconds before getting to the Webpack step.
9. The Webpack compile is kicked off and the CachePlugin runs before the compile, saving the mtime from X.js for later use (it saves 12:00pm and 2 seconds)
10. Webpack then sees if X.js needs to rebuild, and to do so compares the files's mtime, 12:00pm and 2 seconds, and the buildTimestamp, which was set to 12:00pm and 5 seconds. Since the buildTimestamp is greater than that saved mtime, needRebuild returns false. Which is incorrect because the file has been modified.

‡: The main reason why a file mtime is not sufficient for a needRebuild check, is because when the filesystem only has second-level precision you can pretty easily modify & build a file multiple times in the same second. So without anything else, needRuild has to return newMtime >= oldTime—in other words if the newMtime == oldMime, you have to assume that there might be a change. When you check the file's content hashes too (only if the newMtime >= oldMtime) you can be sure that you are not unnecessarily rebuilding file. (I created these tests to check for this sort of unnecessary rebuilding)