Skip to content

[node-modules] Fixes race between mkdirp and remove #1108

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 23, 2020
Merged

[node-modules] Fixes race between mkdirp and remove #1108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d6da2f
Fixes race between `mkdirp` and `remove` during module linking by doi…
larixer Mar 23, 2020
b78ca4d
Get rid of keepNodeModules by keeping inner node_modules by default d…
larixer Mar 23, 2020
30f6a6e
Add assertion check that removed directory has node_modules
larixer Mar 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .yarn/versions/37861a83.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
releases:
"@yarnpkg/cli": prerelease
"@yarnpkg/plugin-node-modules": prerelease

declined:
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/core"
- "@yarnpkg/doctor"
83 changes: 49 additions & 34 deletions packages/plugin-node-modules/sources/NodeModulesLinker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,10 @@ function getLocationMap(installState: NodeModulesLocatorMap) {
return locationMap;
}

const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean, excludeNodeModules?: boolean}): Promise<any> => {
const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean}): Promise<any> => {
if (dir.split(ppath.sep).indexOf(NODE_MODULES) < 0)
throw new Error(`Assertion failed: trying to remove dir that doesn't contain node_modules: ${dir}`);

try {
if (!options || !options.innerLoop) {
const stats = await xfs.lstatPromise(dir);
Expand All @@ -330,7 +333,7 @@ const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean, excl
for (const entry of entries) {
const targetPath = ppath.join(dir, toFilename(entry.name));
if (entry.isDirectory()) {
if (entry.name !== NODE_MODULES || !options || !options.excludeNodeModules) {
if (entry.name !== NODE_MODULES || (options && options.innerLoop)) {
await removeDir(targetPath, {innerLoop: true});
}
} else {
Expand Down Expand Up @@ -447,7 +450,7 @@ const buildLocationTree = (locatorMap: NodeModulesLocatorMap | null, {skipPrefix
const symlinkPromise = async (srcDir: PortablePath, dstDir: PortablePath) =>
xfs.symlinkPromise(process.platform !== 'win32' ? ppath.relative(ppath.dirname(dstDir), srcDir) : srcDir, dstDir, process.platform === 'win32' ? 'junction' : undefined);

const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs}: {baseFs: FakeFS<PortablePath>}) => {
const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs, innerLoop}: {baseFs: FakeFS<PortablePath>, innerLoop?: boolean}) => {
await xfs.mkdirpPromise(dstDir);
const entries = await baseFs.readdirPromise(srcDir, {withFileTypes: true});

Expand All @@ -472,7 +475,9 @@ const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs}:
const srcPath = ppath.join(srcDir, toFilename(entry.name));
const dstPath = ppath.join(dstDir, toFilename(entry.name));
if (entry.isDirectory()) {
await copyPromise(dstPath, srcPath, {baseFs});
if (entry.name !== NODE_MODULES || innerLoop) {
await copyPromise(dstPath, srcPath, {baseFs, innerLoop: true});
}
} else {
await copy(dstPath, srcPath, entry);
}
Expand Down Expand Up @@ -574,10 +579,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
const locationTree = buildLocationTree(installState, {skipPrefix: project.cwd});

const addQueue: Promise<void>[] = [];
const addModule = async ({srcDir, dstDir, linkType, keepNodeModules}: {srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType, keepNodeModules: boolean}) => {
const addModule = async ({srcDir, dstDir, linkType}: {srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType}) => {
const promise: Promise<any> = (async () => {
try {
await removeDir(dstDir, {excludeNodeModules: keepNodeModules});
if (linkType === LinkType.SOFT) {
await xfs.mkdirpPromise(ppath.dirname(dstDir));
await symlinkPromise(ppath.resolve(srcDir), dstDir);
Expand All @@ -597,14 +601,12 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
}
};

const cloneModule = async (srcDir: PortablePath, dstDir: PortablePath, options?: { keepSrcNodeModules?: boolean, keepDstNodeModules?: boolean, innerLoop?: boolean }) => {
const cloneModule = async (srcDir: PortablePath, dstDir: PortablePath, options?: { innerLoop?: boolean }) => {
const promise: Promise<any> = (async () => {
const cloneDir = async (srcDir: PortablePath, dstDir: PortablePath, options?: { keepSrcNodeModules?: boolean, keepDstNodeModules?: boolean, innerLoop?: boolean }) => {
const cloneDir = async (srcDir: PortablePath, dstDir: PortablePath, options?: { innerLoop?: boolean }) => {
try {
if (!options || !options.innerLoop) {
await removeDir(dstDir, {excludeNodeModules: options && options.keepDstNodeModules});
if (!options || !options.innerLoop)
await xfs.mkdirpPromise(dstDir);
}

const entries = await xfs.readdirPromise(srcDir, {withFileTypes: true});
for (const entry of entries) {
Expand All @@ -614,13 +616,13 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
const src = ppath.join(srcDir, entry.name);
const dst = ppath.join(dstDir, entry.name);

if (entry.name !== NODE_MODULES || !options || !options.keepSrcNodeModules) {
if (entry.isDirectory()) {
if (entry.isDirectory()) {
if (entry.name !== NODE_MODULES || (options && options.innerLoop)) {
await xfs.mkdirpPromise(dst);
await cloneDir(src, dst, {keepSrcNodeModules: false, keepDstNodeModules: false, innerLoop: true});
} else {
await xfs.copyFilePromise(src, dst, fs.constants.COPYFILE_FICLONE);
await cloneDir(src, dst, {innerLoop: true});
}
} else {
await xfs.copyFilePromise(src, dst, fs.constants.COPYFILE_FICLONE);
}
}
} catch (e) {
Expand Down Expand Up @@ -661,39 +663,53 @@ async function persistNodeModules(preinstallState: InstallState, installState: N


// Delete locations that no longer exist
const deleteList: PortablePath[] = [];
const deleteList = new Set<PortablePath>();
// Delete locations of inner node_modules
const innerDeleteList = new Set<PortablePath>();
for (const {locations} of preinstallState.locatorMap.values()) {
for (const location of locations) {
const {locationRoot, segments} = parseLocation(location, {
skipPrefix: project.cwd,
});

let prevNode = prevLocationTree.get(locationRoot);
let node = locationTree.get(locationRoot);
let curLocation = locationRoot;
if (!node) {
deleteList.push(curLocation);
} else {
if (node) {
for (const segment of segments) {
// '.' segment exists only for top-level locator, skip it
if (segment === '.')
continue;

curLocation = ppath.join(curLocation, segment);
node = node.children.get(segment);
if (prevNode)
prevNode = prevNode.children.get(segment);

if (!node) {
deleteList.push(curLocation);
deleteList.add(curLocation);
// If previous install had inner node_modules folder, we should explicitely list it for
// `removeDir` to delete it, but we need to delete it first, so we add it to inner delete list
if (prevNode && prevNode.children.has(NODE_MODULES))
innerDeleteList.add(ppath.join(curLocation, NODE_MODULES));
Comment on lines +690 to +694
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be enough to process the delete list in the reverse insertion order?

Copy link
Member Author

@larixer larixer Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since we run all delete operations in parallel

break;
}
}
}
}
}

// Handle inner node_modules deletions first
for (const dstDir of innerDeleteList)
await deleteModule(dstDir);
await Promise.all(deleteQueue);
deleteQueue.length = 0;

for (const dstDir of deleteList)
await deleteModule(dstDir);

// Update changed locations
const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType, keepNodeModules: boolean}> = [];
const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType}> = [];
for (const [prevLocator, {locations}] of preinstallState.locatorMap.entries()) {
for (const location of locations) {
const {locationRoot, segments} = parseLocation(location, {
Expand All @@ -715,9 +731,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
const srcDir = info.target;
const dstDir = curLocation;
const linkType = info.linkType;
const keepNodeModules = node.children.size > 0;
if (srcDir !== dstDir) {
addList.push({srcDir, dstDir, linkType, keepNodeModules});
deleteList.add(dstDir);
addList.push({srcDir, dstDir, linkType});
}
}
}
Expand Down Expand Up @@ -747,13 +763,15 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
node = node!.children.get(segment);

if (!prevTreeNode) {
addList.push({srcDir, dstDir, linkType, keepNodeModules: node!.children.size > 0});
deleteList.add(dstDir);
addList.push({srcDir, dstDir, linkType});
} else {
for (const segment of segments) {
curLocation = ppath.join(curLocation, segment);
prevTreeNode = prevTreeNode.children.get(segment);
if (!prevTreeNode) {
addList.push({srcDir, dstDir, linkType, keepNodeModules: node!.children.size > 0});
deleteList.add(dstDir);
addList.push({srcDir, dstDir, linkType});
break;
}
}
Expand All @@ -765,18 +783,15 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
const reportedProgress = report.reportProgress(progress);

try {
const persistedLocations = new Map<PortablePath, {
dstDir: PortablePath,
keepNodeModules: boolean,
}>();
const persistedLocations = new Map<PortablePath, PortablePath>();

// For the first pass we'll only want to install a single copy for each
// source directory. We'll later use the resulting install directories for
// the other instances of the same package (this will avoid us having to
// crawl the zip archives for each package).
for (const entry of addList) {
if (entry.linkType === LinkType.SOFT || !persistedLocations.has(entry.srcDir)) {
persistedLocations.set(entry.srcDir, {dstDir: entry.dstDir, keepNodeModules: entry.keepNodeModules});
persistedLocations.set(entry.srcDir, entry.dstDir);
await addModule({...entry});
}
}
Expand All @@ -787,9 +802,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N

// Second pass: clone module duplicates
for (const entry of addList) {
const locationInfo = persistedLocations.get(entry.srcDir)!;
if (entry.linkType !== LinkType.SOFT && entry.dstDir !== locationInfo.dstDir) {
await cloneModule(locationInfo.dstDir, entry.dstDir, {keepSrcNodeModules: locationInfo.keepNodeModules, keepDstNodeModules: entry.keepNodeModules});
const persistedDir = persistedLocations.get(entry.srcDir)!;
if (entry.linkType !== LinkType.SOFT && entry.dstDir !== persistedDir) {
await cloneModule(persistedDir, entry.dstDir);
}
}

Expand Down