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

Return success from FileManager recursive directory creation even if a directory was created by another thread concurrently. #2700

Merged
merged 1 commit into from Jun 18, 2020

Conversation

fumoboy007
Copy link
Contributor

If another thread creates one of the same directories in the target path, the !fileExists check may pass while the mkdir call fails with EEXIST. However, if the existing file is a directory, then that should still be a success.

Fixes SR-12272.

@spevans
Copy link
Collaborator

spevans commented Feb 27, 2020

@swift-ci test linux

throw _NSErrorWithErrno(errno, reading: false, path: path)
} else if let attr = attributes {
let posixError = errno
if posixError == EEXIST && fileExists(atPath: path, isDirectory: &isDir) && isDir.boolValue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the test for EEXIST is only needed on the last path component to check if is a file.

If an earlier mkdir() fails because the existing path is a file there should be an ENOTDIR error returned, which should also handle the edge case of path being a directory when fileExists is called but being replaced by a file before the next mkdir().

Would need an extra test with a pre-existing path with includes a file as the last component to check this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EEXIST is returned by mkdir if the last path component already exists, regardless of whether it is a file or a directory. ENOTDIR is returned if any of the path components aside from the last one is not a directory.

This is supported by the documentation and by the example in SR-12272, which shows mkdir returning EEXIST even though there are no files involved.

Copy link
Contributor

@millenomi millenomi left a comment

Choose a reason for hiding this comment

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

@swift-ci please test linux

@fumoboy007
Copy link
Contributor Author

fumoboy007 commented Mar 22, 2020

@millenomi Is the test job stuck or something? How come it has not run yet?

@millenomi
Copy link
Contributor

Sometimes the traffic on CI is so high that requests get dropped.

@millenomi
Copy link
Contributor

@swift-ci please test and merge

1 similar comment
@millenomi
Copy link
Contributor

@swift-ci please test and merge

@spevans
Copy link
Collaborator

spevans commented Apr 7, 2020

@swift-ci test linux

@xwu
Copy link
Collaborator

xwu commented Apr 12, 2020

@swift-ci test macOS

…f a directory was created by another thread concurrently.

If another thread creates one of the same directories in the target path, the `!fileExists` check may pass while the `mkdir` call fails with `EEXIST`. However, if the existing file is a directory, then that should still be a success.

Fixes [SR-12272](https://bugs.swift.org/browse/SR-12272).
@fumoboy007
Copy link
Contributor Author

Resolved the conflict. Please re-trigger the tests.

@spevans
Copy link
Collaborator

spevans commented Jun 11, 2020

@swift-ci please test

@fumoboy007
Copy link
Contributor Author

Yay, passed! Can we merge?

@spevans
Copy link
Collaborator

spevans commented Jun 18, 2020

@swift-ci please test and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants