-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
base: main
Are you sure you want to change the base?
Conversation
lib/internal/fs/cp/cp.js
Outdated
await mkdir(dest); | ||
} catch (error) { | ||
// If the folder already exists, skip it. | ||
if (error.code !== 'EEXIST') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
3ae92fb
to
5659000
Compare
@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? |
try { | ||
await mkdir(dest); | ||
} catch (error) { | ||
// If the folder already exists, skip it. | ||
if (error.code === 'EEXIST' && !opts.errorOnExist); else | ||
throw error; | ||
} |
There was a problem hiding this comment.
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
.
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 | |
}); |
PTAL @anonrig. I just encountered the same issue. When using fs.cp in a concurrent situation, this bug occurs. |
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]>
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]>
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.
The relevant code is in lib/internal/fs/cp/cp.js.
destStat is empty here, it did not exist when checking the folder before.
I want to ignore the EEXIST error thrown by mkdir