search

jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.44k stars 776 forks source link

copy does not correctly overwrite due to missing mkdir recursive true #964

Closed kroeder closed 2 years ago

kroeder commented 2 years ago

I was investigating https://github.com/storybookjs/storybook/issues/16732 and debugged my way to fs-extra "copy"

A summary of my investigation for Storybook can be found in the comments

I tested it with the version that Storybook installs in the latest 6.5 release (9.1.0) and also forced Storybook to use 10.1.0 Both ended in the same error

Storybook uses fs-extra to copy assets from a dev-directory to a build directory (dist) Unfortunately, I could not reproduce it with a test file in this repo

Whats happening?

Storybook maps an object like this 

    staticDirs: [
        {
            from: '../../../src/assets/i18n',
            to: 'i18n',
        },
        {
            from: '../../../projects/ui/src/assets/i18n',
            to: 'i18n/ui',
        },
    ],

to the copy command in fs-extra (https://github.com/storybookjs/storybook/blob/7035ea7389393da041985ebc491ee58dedb50d06/code/lib/core-server/src/utils/copy-all-static-files.ts#L55)

    await fs.copy(from, targetPath, {
      dereference: true,
      preserveTimestamps: true,
      filter: (_, dest) => !skipPaths.includes(dest),
    });

When running the Storybook build, the following error appears

[Error: EEXIST: file already exists, mkdir 'C:\Users\kro\Development\next-client\dist\storybook\storybook\i18n'] {
  errno: -4075,
  code: 'EEXIST',
  syscall: 'mkdir',
  path: 'C:\\Users\\kro\\Development\\next-client\\dist\\storybook\\storybook\\i18n'
}

This made no sense to me since the overwrite flag is true by default. I did some more research and finally debugged my way to the mkdir call that causes the error as soon as it runs "mkdir" the second time using the above staticDirs object

This is the code that throws the error

https://github.com/jprichardson/node-fs-extra/blob/9.1.0/lib/copy/copy.js#L152-L160

However, if I set recursive: true, the error disappears and the result looks as expected.

Any chance there is something missing like recursive: opts.overwrite?

Also, in the Storybook issue, some claim that this is probably a breaking change from node 14 to node 16 (https://github.com/storybookjs/storybook/issues/16732#issuecomment-1055245540)

Any chance you know what's going on here? Happy to help with further Information! This was reproduced on macOS and windows both using node 16

RyanZim commented 2 years ago

However, if I set recursive: true, the error disappears and the result looks as expected.

fs.mkdir with recursive: true silences the error if the directory already exists. However, fs-extra is only attempting to call fs.mkdir if the destination directory does not exist. The question is how does fs-extra think the directory doesn't exist, but then it actually exists when we go to create the directory?

I would suspect the answer lies in the fact that you're copying to: 'i18n' and to: 'i18n/ui' (both the parent directory and the subdirectory). Looking at the storybook code, these staticDirs are being handled by a forEach: https://github.com/storybookjs/storybook/blob/7035ea7389393da041985ebc491ee58dedb50d06/code/lib/core-server/src/utils/copy-all-static-files.ts#L37

This means each of these directories are being copied in parallel, since forEach does not wait for an async callback to finish/resolve before proceeding to the next item. So you have one copy trying to copy to i18n, and another copy running at the same time, trying to copy to i18n/ui. They both see that i18n doesn't exist, so they both try to create it. Of course, that's a race condition, because one of the copy operations actually creates i18n before the other, and this other copy operation then gets an error, because the directory already exists. You can confirm this by removing one of those staticDir entries; I suspect you'll get no error. Please confirm this before we move forward.

As an aside, using forEach with an async callback is a bad practice. copyAllStaticFilesRelativeToMain is an async function, but it's not actually awaiting anything; it will return synchronously, before any files have begun to be copied. Additionally, you're not getting normal error handling, since none of the errors in that callback will bubble up to copyAllStaticFilesRelativeToMain. await Promise.all(staticDirs.map(async (dir) => ... would be the correct approach here.

kroeder commented 2 years ago

Thanks a lot for this fast and in-depth feedback! I'll take a look at what you wrote and get back with an update later

kroeder commented 2 years ago

I can confirm, it's a bug in Storybook, thanks for your help 🙂

RyanZim commented 2 years ago

Not a problem, happy to help. And thank you for your well-written, in-depth issue that gave me all the information I needed up front; much appreciated!