fix: Patch pkg to fix race condition in mkdir#2312
Merged
dividedmind merged 2 commits intomainfrom May 22, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a race condition in temporary directory creation for native node addon loading by replacing a custom recursive directory creation function with the built-in recursive mkdir feature in Node.js.
- Replace custom createDirRecursively() with fs.mkdirSync(tmpFolder, { recursive: true })
- Update package.json to apply a patch for pkg version 5.8.1
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| package.json | Updated resolutions to include the patched pkg version |
| .yarn/patches/pkg-npm-5.8.1-db9700609f.patch | Replaced the custom recursive directory creation function with fs.mkdirSync with recursive enabled |
dustinbyrne
approved these changes
May 22, 2025
Replace custom recursive directory creation with built-in recursive mkdir in node addon loading. When multiple packaged applications try to start simultaneously, they can encounter race conditions when creating temporary directories for loading native node addons. This manifests as an "EEXIST" error when calling `mkdirSync()`. The issue occurs in the native module loading code path where pkg needs to extract native addons to a temporary location before they can be loaded via `process.dlopen()`. The current implementation uses a custom `createDirRecursively()` function that has a race condition - it checks if a directory exists and then tries to create it, but another process could create the directory between the check and creation. Node.js has built-in support for recursive directory creation via the `recursive: true` option in `mkdirSync()`. This handles race conditions properly - if the directory already exists, it will not throw an error. This is exactly what we need.
30e1f0f to
987fda0
Compare
Collaborator
Author
|
Here's the upstream PR for reference: yao-pkg/pkg#154 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace custom recursive directory creation with built-in recursive mkdir in node addon loading
When multiple packaged applications try to start simultaneously, they can encounter race conditions when creating temporary directories for loading native node addons. This manifests as an "EEXIST" error when calling
mkdirSync().The issue occurs in the native module loading code path where pkg needs to extract native addons to a temporary location before they can be loaded via
process.dlopen(). The current implementation uses a customcreateDirRecursively()function that has a race condition - it checks if a directory exists and then tries to create it, but another process could create the directory between the check and creation.Node.js has built-in support for recursive directory creation via the
recursive: trueoption inmkdirSync(). This handles race conditions properly - if the directory already exists, it will not throw an error. This is exactly what we need.