ts-patch icon indicating copy to clipboard operation
ts-patch copied to clipboard

[Feature] Add mutex to `ts-patch install`

Open salieri opened this issue 2 years ago • 2 comments

I'm using ts-patch in a monorepo with rushjs and pnpm. When installing multiple projects (=multiple package.json files) at once, the prepare script calling ts-patch install -s in each of the package.json files can cause havoc. PNPM does magic with symlinks, so many projects will share the same dependency files.

In summary, the PNPM installation goes something like this:

  1. Download and install all dependencies across all projects in the monorepo
  2. Run prepare script on all projects in the monorepo at the same time (=parallel execution)

This leads to incidents where the patch to node_modules/typescript is applied incorrectly or partially. ts-patch has a check for already patched files, but if two prepare scripts run simultaneously, that check can be missed.

In order to fix this reliably, I applied a mutex to all the prepare scripts in my monorepo, changing the prepare from this:

{
  "scripts": {
    "prepare": "ts-patch install -s"
  }
}

To this:

{
  "scripts": {
    "prepare": "acquire-mutex && ts-patch install -s && release-mutex"
  }
}

That has fixed the patch issues for me. Ideally the mutex should be part of ts-patch though, so everyone using PNPM/Rush.js wouldn't have to write their own mutex scripts.

Error example:

/path/to/common/temp/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/tsc.js:117780
ts.diagnosticMap.set(program, allDiagnostics);

TypeError: Cannot read properties of undefined (reading 'set')

salieri avatar Apr 23 '22 21:04 salieri

Great point. Thanks for raising this.

The main option with node would be to use file locking, but it turns out this is a little tricky in terms of multi-OS support. fs-ext added support as of Node ~ 8.9, but there were various recent bug reports of it not working on later node, and it also requires python + node-gyp just to install, as it goes through a build process. If you're missing one of these components, it throws a nasty error when you try to install. Unfortunately, this makes it not a viable choice.

What I ended up doing was simply creating a temporary file in the destination directory (the specific ts package path). With this route, we essentially indicate that this ts install path is "locked". I also added logic so that it will retry up to 3 times with 10sec pause. This should alleviate any issues.

While we do have a proper finally block which ensures it gets "unlocked", I've also added a --force flag which will skip the mutex check in case the file doesn't get deleted in some cases. The final throw after 3 attempts indicates this, as well.

That said, I think the bases are pretty well covered in this approach. Feel free to weigh in if you have any thoughts.

All that is left to do is add in some proper tests, and I'll get a release together.

nonara avatar Apr 24 '22 00:04 nonara

Is there any progress in this matter ?

wangkailang avatar Jun 24 '22 04:06 wangkailang

Added in v3

nonara avatar Jun 13 '23 22:06 nonara