Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add mutex to ts-patch install #75

Closed
salieri opened this issue Apr 23, 2022 · 3 comments · Fixed by #96
Closed

[Feature] Add mutex to ts-patch install #75

salieri opened this issue Apr 23, 2022 · 3 comments · Fixed by #96

Comments

@salieri
Copy link

salieri commented Apr 23, 2022

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')
@nonara nonara changed the title Add mutex to ts-patch install [Feature] Add mutex to ts-patch install Apr 24, 2022
@nonara
Copy link
Owner

nonara commented Apr 24, 2022

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.

@wangkailang
Copy link

Is there any progress in this matter ?

@nonara nonara mentioned this issue Jun 13, 2023
Merged
nonara added a commit that referenced this issue Jun 13, 2023
- Added Live Compiler (on-the-fly, in-memory patching)
- Added experimental ES Module support (closes #58)
- Added mutex locks (closes #75)
- Updated to support TS v5+ (closes #83 closes #93)
- Fixed patching for non-standard libraries (cannot guarantee they will work as expected in IDEs) (closes #85)
- Added caching
@nonara
Copy link
Owner

nonara commented Jun 13, 2023

Added in v3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants