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

fs: calling mkdir in fs.cp function can ignore EEXIST error #53534

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ShenHongFei
Copy link
Contributor

This PR mainly aims to solve the following problem. During the execution of the fsp.cp function, due to other file operations, the target directory of fsp.cp is created in parallel. The actual situation may be more complicated. Multiple file operations are executed concurrently, resulting in an error in the internal fsp.cp when calling mkdir, because the folder has been created by other file operations. In this case, you can actually ignore this error and continue with the subsequent file copying operation.

import { promises as fsp } from 'fs'

const src = 'T:/src/'
const dst = 'T:/out/'

try {
    await fsp.rm(dst, { recursive: true })
} catch { }

await Promise.all([
    fsp.cp(src, dst, {
        recursive: true,
        force: true,
        errorOnExist: false,
        mode: 0
    }),
    
    // example other file operations in parallel
    fsp.mkdir(dst, { recursive: true })
])

// throws EEXIST: file already exists, mkdir 'T:/out/'
// at mkdir()
// at mkDirAndCopy() lib/internal/fs/cp/cp.js
// at onDir() lib/internal/fs/cp/cp.js

The relevant code is in lib/internal/fs/cp/cp.js.
destStat is empty here, it did not exist when checking the folder before.

function onDir(srcStat, destStat, src, dest, opts) {
  if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts);
  return copyDir(src, dest, opts);
}

async function mkDirAndCopy(srcMode, src, dest, opts) {
  await mkdir(dest);
  await copyDir(src, dest, opts);
  return setDestMode(dest, srcMode);
}

I want to ignore the EEXIST error thrown by mkdir

async function mkDirAndCopy(srcMode, src, dest, opts) {
  try {
    await mkdir(dest);
  } catch (error) {
    // If the folder already exists, skip it.
    if (error.code !== 'EEXIST')
      throw error;
  }

  await copyDir(src, dest, opts);
  return setDestMode(dest, srcMode);
}

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 21, 2024
await mkdir(dest);
} catch (error) {
// If the folder already exists, skip it.
if (error.code !== 'EEXIST')
Copy link
Member

Choose a reason for hiding this comment

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

This pull-request needs a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test and adjusted it to ignore this error only when the errorOnExist parameter is false

@ShenHongFei ShenHongFei requested a review from anonrig June 24, 2024 04:31
@ShenHongFei ShenHongFei force-pushed the fs-cp-mkdir-ignore-eexist branch from 3ae92fb to 5659000 Compare June 24, 2024 05:43
@ShenHongFei
Copy link
Contributor Author

@anonrig I added a test and changed the logic to ignore the error only when the errorOnExist parameter is false. What else do I need to do? Can you take another look?

Comment on lines +309 to +315
try {
await mkdir(dest);
} catch (error) {
// If the folder already exists, skip it.
if (error.code === 'EEXIST' && !opts.errorOnExist); else
throw error;
}

Choose a reason for hiding this comment

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

When set to recursive, it will automatically ignore the EEXIST error. And the situation that needs to consider force.

Suggested change
try {
await mkdir(dest);
} catch (error) {
// If the folder already exists, skip it.
if (error.code === 'EEXIST' && !opts.errorOnExist); else
throw error;
}
await mkdir(dest, {
recursive: opts.force || !opts.errorOnExist
});

@BlackHole1
Copy link

PTAL @anonrig. I just encountered the same issue. When using fs.cp in a concurrent situation, this bug occurs.

BlackHole1 added a commit to oomol-lab/oopm that referenced this pull request Feb 12, 2025
When installing duplicated dependencies, it triggers concurrency issues in Node.js (see: nodejs/node#53534).

This can be avoided by pre-deduplication.

Signed-off-by: Kevin Cui <[email protected]>
l1shen pushed a commit to oomol-lab/oopm that referenced this pull request Feb 12, 2025
When installing duplicated dependencies, it triggers concurrency issues
in Node.js (see: nodejs/node#53534).

This can be avoided by pre-deduplication.

Signed-off-by: Kevin Cui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants