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.

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions Sources/Foundation/FileManager+POSIX.swift
View file
Open in desktop
Expand Up @@ -346,8 +346,16 @@ extension FileManager {
try createDirectory(atPath: parent, withIntermediateDirectories: true, attributes: attributes)
}
if mkdir(pathFsRep, S_IRWXU | S_IRWXG | S_IRWXO) != 0 {
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.

// Continue; if there is an existing file and it is a directory, that is still a success.
// There can be an existing file if another thread or process concurrently creates the
// same file.
} else {
throw _NSErrorWithErrno(posixError, reading: false, path: path)
}
}
if let attr = attributes {
try self.setAttributes(attr, ofItemAtPath: path)
}
} else if isDir.boolValue {
Expand Down
46 changes: 46 additions & 0 deletions Tests/Foundation/Tests/TestFileManager.swift
View file
Open in desktop
Expand Up @@ -15,6 +15,8 @@
#endif
#endif

import Dispatch

class TestFileManager : XCTestCase {
#if os(Windows)
let pathSep = "\\"
Expand Down Expand Up @@ -1870,6 +1872,49 @@ VIDEOS=StopgapVideos
try checkPath(path: path)
}
}

/**
Tests that we can get an item replacement directory concurrently.

- Bug: [SR-12272](https://bugs.swift.org/browse/SR-12272)
*/
func test_concurrentGetItemReplacementDirectory() throws {
let fileManager = FileManager.default

let operationCount = 10

var directoryURLs = [URL?](repeating: nil, count: operationCount)
var errors = [Error?](repeating: nil, count: operationCount)

let dispatchGroup = DispatchGroup()
for operationIndex in 0..<operationCount {
DispatchQueue.global().async(group: dispatchGroup) {
do {
let directory = try fileManager.url(for: .itemReplacementDirectory,
in: .userDomainMask,
appropriateFor: URL(fileURLWithPath: NSTemporaryDirectory(),
isDirectory: true),
create: true)
directoryURLs[operationIndex] = directory
} catch {
errors[operationIndex] = error
}
}
}
dispatchGroup.wait()

for directoryURL in directoryURLs {
if let directoryURL = directoryURL {
try? fileManager.removeItem(at: directoryURL)
}
}

for error in errors {
if let error = error {
XCTFail("One of the concurrent calls to get the item replacement directory failed: \(error)")
}
}
}

// -----

Expand Down Expand Up @@ -1928,6 +1973,7 @@ VIDEOS=StopgapVideos
("test_contentsEqual", test_contentsEqual),
/* ⚠️ */ ("test_replacement", testExpectedToFail(test_replacement,
/* ⚠️ */ "<https://bugs.swift.org/browse/SR-10819> Re-enable Foundation test TestFileManager.test_replacement")),
("test_concurrentGetItemReplacementDirectory", test_concurrentGetItemReplacementDirectory),
]

#if !DEPLOYMENT_RUNTIME_OBJC && NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT && !os(Android)
Expand Down