Serialize execution of the same script across different Wireit processes #189
Conversation
aomarks
requested review from
rictic,
justinfagnani,
AndrewJakubowicz and
augustjk
README.md
Outdated
| @@ -198,6 +198,12 @@ export WIREIT_PARALLEL=1 | |||
| npm run build | |||
| ``` | |||
|
|
|||
| If two or more seperate `npm run` commands are run for the same Wireit script | |||
| simultaneously, then only one version will be allowed to run at a time, while | |||
There was a problem hiding this comment.
| simultaneously, then only one version will be allowed to run at a time, while | |
| simultaneously, then only one instance will be allowed to run at a time, while |
?
Talking about "versions" around npm could potentially be confused with package versions.
| loggedLocked = true; | ||
| } | ||
| // Wait a moment before attempting to acquire the lock again. | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); |
There was a problem hiding this comment.
nit: might be nice to pull these three numeric constants here into named constants.
There was a problem hiding this comment.
I dunno, sometimes I feel like that splits apart code that makes more sense together. Why is it better to need to look somewhere else in the file to find out what the number is?
src/test/parallelism.test.ts
Outdated
| const exec1 = rig.exec('npm run a'); | ||
| const exec2 = rig.exec('npm run a'); | ||
| const inv1 = await cmdA.nextInvocation(); | ||
| const inv2 = await cmdA.nextInvocation(); | ||
| inv1.exit(0); | ||
| inv2.exit(0); | ||
| assert.equal((await exec1.exit).code, 0); | ||
| assert.equal((await exec2.exit).code, 0); | ||
| assert.equal(cmdA.numInvocations, 2); |
There was a problem hiding this comment.
I'm a little lost on how this tests that scripts are running without exclusive lock. Wouldn't this pass even if they ran one after another? Would asserting numInvocations to 2 before exiting be appropriate?
There was a problem hiding this comment.
This is testing the output=[] case, when there is no lock. This test would fail if we did lock, because the 2nd invocation couldn't otherwise start before the 1st one ended.
Added comment.
src/test/util/uvu-timeout.ts
Outdated
| const DEFAULT_TIMEOUT = Number( | ||
| process.env.TEST_TIMEOUT ?? (IS_WINDOWS ? 60_000 : 30_000) | ||
| ); |
There was a problem hiding this comment.
Super nit: I'm guessing Windows specifically needed more time for some of these. Is there harm in just changing the default globally to 60? Or can we set the TEST_TIMEOUT environment variable for windows tests? The conditional default value in the code just seems kinda weird.
There was a problem hiding this comment.
Yeah, one of these needed more time on Windows. I've seen some other tests occasionally time out on windows too, so I figured I'd just do it universally -- but you're right it's probably better to keep the other ones at 30 seconds, since that gives faster results when there are genuine test failures. And we can just add more overrides the next time we see a timeout.
- Bump to latest version of Wireit to bring in google/wireit#189, which makes it safe to have multiple concurrent Wireit processes handling overlapping build graphs. - Added `output:[]` to some server commands, so that they can run concurrently given the above new restriction. - Renamed `:not-site` scripts to `:assets`, and `:site` scripts to `:eleventy`. I think these are more intuitive names. - Separated out `dev:build:assets` and `prod:build:assets` scripts. This means that watching dev no longer runs rollup, which was unnecessary because we serve tsc output directly in dev mode. - Updated the top-level `dev` script: - Ensure that the server is built first - Simplified to take advantage of safe concurrent Wireit processes I'll be working on google/wireit#33 next, which should simplify things even further.
Previously, if you
npm runmultiple versions of the same script (or 2 Wireit scripts that share a dependency) at the same time, then they would simply run simultaneously, and incorrect output could be produced. In particular, the second Wireit process might try and delete output, while the first one is still writing it.Now, we prevent multiple versions of the same script from running at the same time. Before any script starts, we try to acquire an exclusive lock for it, and we wait if the lock can't be acquired, logging this message:
An exception is when
output=[], because I think these cases are unlikely to cause problems, and it allows users to continue running e.g. multiple versions of the same server or other non-build step if they need to.The lockfile is managed using the proper-lockfile library, which seems to have a nice design. It works by attempting to
mkdira directory, which will either succeed if it doesn't exist, or fail it does exist, atomically. It also detects when a lock has probably been abandoned, by regularly updating the directorymtimewhen the lock is held, and deleting the lock directory if it hasn't been updated in a long time.Fixes #35