From 988c6ee0cd9f37c2c988c1da027bfa62f3daed89 Mon Sep 17 00:00:00 2001 From: Matt Hope Date: Tue, 30 Jul 2024 15:01:36 +0100 Subject: [PATCH 1/4] Provide a default CopyStrategy overload for copyItem. This makes it easier to use, especially when copying files such that it is irrelevant. --- .../NIOFileSystem/FileSystemProtocol.swift | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/Sources/NIOFileSystem/FileSystemProtocol.swift b/Sources/NIOFileSystem/FileSystemProtocol.swift index d60cdd0851..349a5eedc1 100644 --- a/Sources/NIOFileSystem/FileSystemProtocol.swift +++ b/Sources/NIOFileSystem/FileSystemProtocol.swift @@ -535,6 +535,52 @@ extension FileSystemProtocol { } ) } + + /// Copies the item at the specified path to a new location. + /// + /// The following error codes may be thrown: + /// - ``FileSystemError/Code-swift.struct/notFound`` if the item at `sourcePath` does not exist, + /// - ``FileSystemError/Code-swift.struct/invalidArgument`` if an item at `destinationPath` + /// exists prior to the copy or its parent directory does not exist. + /// + /// Note that other errors may also be thrown. + /// + /// If `sourcePath` is a symbolic link then only the link is copied. The copied file will + /// preserve permissions and any extended attributes (if supported by the file system). + /// + /// - Parameters: + /// - sourcePath: The path to the item to copy. + /// - destinationPath: The path at which to place the copy. + /// - shouldProceedAfterError: A closure which is executed to determine whether to continue + /// copying files if an error is encountered during the operation. See Errors section for full details. + /// - shouldCopyItem: A closure which is executed before each copy to determine whether each + /// item should be copied. See Filtering section for full details + /// + /// #### Parallelism + /// + /// This overload uses ``CopyStrategy/platformDefault`` which is likely to result in multiple concurrency domains being used + /// in the event of copying a directory. + /// See the detailed description on ``copyItem(at:to:strategy:shouldProceedAfterError:shouldCopyItem:)`` + /// for the implications of this with respect to the `shouldProceedAfterError` and `shouldCopyItem` callbacks + func copyItem( + at sourcePath: FilePath, + to destinationPath: FilePath, + shouldProceedAfterError: @escaping @Sendable ( + _ source: DirectoryEntry, + _ error: Error + ) async throws -> Void, + shouldCopyItem: @escaping @Sendable ( + _ source: DirectoryEntry, + _ destination: FilePath + ) async -> Bool + ) async throws { + try await self.copyItem( + at: sourcePath, + to: destinationPath, + strategy: .platformDefault, + shouldProceedAfterError: shouldProceedAfterError, + shouldCopyItem: shouldCopyItem) + } /// Deletes the file or directory (and its contents) at `path`. /// From 86a360745ad116c8027a8fb4d9c9f6c500e80b5a Mon Sep 17 00:00:00 2001 From: Matt Hope Date: Tue, 30 Jul 2024 16:09:08 +0100 Subject: [PATCH 2/4] add public access to extension member --- Sources/NIOFileSystem/FileSystemProtocol.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/NIOFileSystem/FileSystemProtocol.swift b/Sources/NIOFileSystem/FileSystemProtocol.swift index 349a5eedc1..0485ff515d 100644 --- a/Sources/NIOFileSystem/FileSystemProtocol.swift +++ b/Sources/NIOFileSystem/FileSystemProtocol.swift @@ -562,7 +562,7 @@ extension FileSystemProtocol { /// in the event of copying a directory. /// See the detailed description on ``copyItem(at:to:strategy:shouldProceedAfterError:shouldCopyItem:)`` /// for the implications of this with respect to the `shouldProceedAfterError` and `shouldCopyItem` callbacks - func copyItem( + public func copyItem( at sourcePath: FilePath, to destinationPath: FilePath, shouldProceedAfterError: @escaping @Sendable ( From b1400d67c33a2f6af405e6302db2c5e95960c1cc Mon Sep 17 00:00:00 2001 From: Matt Hope Date: Tue, 30 Jul 2024 16:16:54 +0100 Subject: [PATCH 3/4] format fixup --- Sources/NIOFileSystem/FileSystemProtocol.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Sources/NIOFileSystem/FileSystemProtocol.swift b/Sources/NIOFileSystem/FileSystemProtocol.swift index 0485ff515d..7163a88469 100644 --- a/Sources/NIOFileSystem/FileSystemProtocol.swift +++ b/Sources/NIOFileSystem/FileSystemProtocol.swift @@ -535,7 +535,7 @@ extension FileSystemProtocol { } ) } - + /// Copies the item at the specified path to a new location. /// /// The following error codes may be thrown: @@ -579,7 +579,8 @@ extension FileSystemProtocol { to: destinationPath, strategy: .platformDefault, shouldProceedAfterError: shouldProceedAfterError, - shouldCopyItem: shouldCopyItem) + shouldCopyItem: shouldCopyItem + ) } /// Deletes the file or directory (and its contents) at `path`. From 94ed19aab5ccc4fadacf1ed829d8d03768e5ddd9 Mon Sep 17 00:00:00 2001 From: Matt Hope Date: Wed, 31 Jul 2024 16:24:51 +0100 Subject: [PATCH 4/4] Fix flaky cancellation. The cancellation tests were flaky, but only because they exposed a code path where cancellation was not properly respected. I've changed the tests to deterministically hit those points, verified that those fail reliably, then put in a fix that solves it. --- .../Internal/ParallelDirCopy.swift | 2 + .../FileSystemTests.swift | 183 +++++++++++++++--- 2 files changed, 153 insertions(+), 32 deletions(-) diff --git a/Sources/NIOFileSystem/Internal/ParallelDirCopy.swift b/Sources/NIOFileSystem/Internal/ParallelDirCopy.swift index cebfb635bb..d2010b6523 100644 --- a/Sources/NIOFileSystem/Internal/ParallelDirCopy.swift +++ b/Sources/NIOFileSystem/Internal/ParallelDirCopy.swift @@ -134,6 +134,8 @@ extension FileSystem { if let item = item { keepConsuming = !onNextItem(item) } else { + // To accurately propagate the cancellation we must check here too + try Task.checkCancellation() keepConsuming = false } } diff --git a/Tests/NIOFileSystemIntegrationTests/FileSystemTests.swift b/Tests/NIOFileSystemIntegrationTests/FileSystemTests.swift index ac4bb83095..d952c2760f 100644 --- a/Tests/NIOFileSystemIntegrationTests/FileSystemTests.swift +++ b/Tests/NIOFileSystemIntegrationTests/FileSystemTests.swift @@ -14,7 +14,7 @@ #if os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(Linux) || os(Android) import NIOCore -@_spi(Testing) import _NIOFileSystem +@_spi(Testing) @testable import _NIOFileSystem @preconcurrency import SystemPackage import XCTest import NIOConcurrencyHelpers @@ -791,10 +791,49 @@ final class FileSystemTests: XCTestCase { _ copyStrategy: CopyStrategy, line: UInt = #line ) async throws { - let path = try await self.fs.temporaryFilePath() + // Whitebox testing to cover specific scenarios + switch copyStrategy.wrapped { + case let .parallel(maxDescriptors): + // The use of nested directories here allows us to rely on deterministic ordering + // of the shouldCopy calls that are used to trigger the cancel. If we used files then directory + // listing is not deterministic in general and that could make tests unreliable. + // If maxDescriptors gets too high the resulting recursion might result in the test failing. + // At that stage the tests would need some rework, but it's not viewed as likely given that + // the maxDescriptors defaults should remain small. + + // Each dir consumes two descriptors, so this source can cover all scenarios. + let depth = maxDescriptors + 1 + let path = try await self.fs.temporaryFilePath() + try await self.generateDeterministicDirectoryStructure( + root: path, + structure: TestFileStructure.makeNestedDirs(depth) { + .init("dir-\(depth - $0)")! + } + ) + + // This covers cancelling before/at the point we reach the limit. + // If the maxDescriptors is sufficiently low we simply can't trigger + // inside that phase so don't try. + if maxDescriptors >= 4 { + try await testCopyCancelledPartWayThrough(copyStrategy, "early_complete", path) { + $0.path.lastComponent!.string == "dir-0" + } + } + // This covers completing after we reach the steady state phase. + let triggerAt = "dir-\(maxDescriptors / 2 + 1)" + try await testCopyCancelledPartWayThrough(copyStrategy, "late_complete", path) { + $0.path.lastComponent!.string == triggerAt + } + case .sequential: + // nothing much to whitebox test here + break + } + // Keep doing random ones as a sort of fuzzing, it previously highlighted some interesting cases + // that are now covered in the whitebox tests above + let randomPath = try await self.fs.temporaryFilePath() let _ = try await self.generateDirectoryStructure( - root: path, + root: randomPath, // Ensure: // - Parallelism is possible in directory scans. // - There are sub directories underneath the point we trigger cancel @@ -803,6 +842,22 @@ final class FileSystemTests: XCTestCase { directoryProbability: 1.0, symbolicLinkProbability: 0.0 ) + try await testCopyCancelledPartWayThrough( + copyStrategy, + "randomly generated", + randomPath + ) { source in + source.path != randomPath && source.path.removingLastComponent() != randomPath + } + } + + private func testCopyCancelledPartWayThrough( + _ copyStrategy: CopyStrategy, + _ description: String, + _ path: FilePath, + triggerCancel: @escaping (DirectoryEntry) -> Bool, + line: UInt = #line + ) async throws { let copyPath = try await self.fs.temporaryFilePath() @@ -814,7 +869,7 @@ final class FileSystemTests: XCTestCase { throw error } shouldCopyItem: { source, destination in // Abuse shouldCopy to trigger the cancellation after getting some way in. - if source.path != path && source.path.removingLastComponent() != path { + if triggerCancel(source) { let shouldSleep = requestedCancel.withLockedValue { requested in if !requested { requested = true @@ -828,12 +883,12 @@ final class FileSystemTests: XCTestCase { if shouldSleep { do { try await Task.sleep(for: .seconds(3)) - XCTFail("Should have been cancelled by now!") + XCTFail("\(description) Should have been cancelled by now!") } catch is CancellationError { // This is fine - we got cancelled as desired, let the rest of the in flight // logic wind down cleanly (we hope/assert) } catch let error { - XCTFail("just expected a cancellation error not \(error)") + XCTFail("\(description) just expected a cancellation error not \(error)") } } } @@ -854,13 +909,13 @@ final class FileSystemTests: XCTestCase { let result = await task.result switch result { case let .success(msg): - XCTFail("expected the cancellation to have happened : \(msg)") + XCTFail("\(description) expected the cancellation to have happened : \(msg)") case let .failure(err): if err is CancellationError { // success } else { - XCTFail("expected CancellationError not \(err)") + XCTFail("\(description) expected CancellationError not \(err)") } } // We can't assert anything about the state of the copy, @@ -1359,6 +1414,92 @@ extension FileSystemTests { XCTAssertEqual(destination1, destination2) } + /// Declare a directory structure in code to make with ``generateDeterministicDirectoryStructure`` + fileprivate enum TestFileStructure { + case dir(_ name: FilePath.Component, _ contents: [TestFileStructure] = []) + case file(_ name: FilePath.Component) + // don't care about the destination yet + case symbolicLink(_ name: FilePath.Component) + + static func makeNestedDirs( + _ depth: Int, + namer: (Int) -> FilePath.Component = { .init("dir-\($0)")! } + ) -> [TestFileStructure] { + let name = namer(depth) + guard depth > 0 else { + return [] + } + return [.dir(name, makeNestedDirs(depth - 1, namer: namer))] + } + + static func makeManyFiles( + _ num: Int, + namer: (Int) -> FilePath.Component = { .init("file-\($0)")! } + ) -> [TestFileStructure] { + (0..