Add atomic writes to FileSystem and implement it for LocalFileSystem #1617
Conversation
Sources/Basic/FileSystem.swift
Outdated
| } | ||
| let temp = try TemporaryFile(deleteOnClose: false) | ||
| try writeFileContents(temp.path, bytes: bytes) | ||
| try rename(temp.path, to: path) |
There was a problem hiding this comment.
If rename didn't work, need to attempt to delete the temporary file.
Sources/Basic/FileSystem.swift
Outdated
| return try writeFileContents(path, bytes: bytes) | ||
| } | ||
| let temp = try TemporaryFile(deleteOnClose: false) | ||
| try writeFileContents(temp.path, bytes: bytes) |
There was a problem hiding this comment.
If write didn't work, need to attempt to delete the temporary file.
Sources/Basic/TemporaryFile.swift
Outdated
| self.suffix = suffix | ||
| self.prefix = prefix | ||
| self.deleteOnClose = deleteOnClose | ||
| // Determine in which directory to create the temporary file. | ||
| self.dir = determineTempDirectory(dir) |
There was a problem hiding this comment.
Would you please measure the typical cost of this determineTempDirectory on a local filesystem, in microseconds? I have a bad suspicion.
There was a problem hiding this comment.
The first time it actually needs to get the environment variables, it takes about 1ms. From then on, each call takes around 12 microseconds.
|
|
||
| // Atomic writes | ||
| // In-memory file system doesn't have atomic writes implemented, so it throws. | ||
| XCTAssertThrows(FileSystemError.unsupported, { try Basic.InMemoryFileSystem(files: [:]).writeFileContents(filePath, bytes: ByteString(testData), atomically: true) }) |
There was a problem hiding this comment.
With the in-memory file system it should trivial to implement atomic writes, even without going through the temporary file. I think if you write as non-atomic, it'd appear atomic.
There was a problem hiding this comment.
Done, it's now treating the base implementation of in-memory file system's writeFileContents as atomic.
|
@vlm Updated the PR with your feedback, let me know if there's anything else. |
Sources/Basic/FileSystem.swift
Outdated
|
|
||
| func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws { | ||
| // Perform non-atomic writes using the fast path | ||
| guard atomically else { |
There was a problem hiding this comment.
nit: I would prefer using if here.
There was a problem hiding this comment.
Sure, changed to if !atomically {
Sources/Basic/FileSystem.swift
Outdated
| @@ -351,6 +365,24 @@ private class LocalFileSystem: FileSystem { | |||
| break | |||
| } | |||
| } | |||
|
|
|||
| func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws { | |||
| // Perform non-atomic writes using the fast path | |||
There was a problem hiding this comment.
nit: end comments with a punctuation.
Sources/Basic/FileSystem.swift
Outdated
| guard atomically else { | ||
| return try writeFileContents(path, bytes: bytes) | ||
| } | ||
| let temp = try TemporaryFile(deleteOnClose: false) |
There was a problem hiding this comment.
We should create the temporary file in the directory where the actual file needs to be written as it could be expensive to move files from OS's temp directory to final location (which could be a network drive).
There was a problem hiding this comment.
Okay, I've made that change
Sources/Basic/TemporaryFile.swift
Outdated
| @@ -74,6 +74,9 @@ public final class TemporaryFile { | |||
|
|
|||
| /// FileHandle of the temporary file, can be used to read/write data. | |||
| public let fileHandle: FileHandle | |||
|
|
|||
| /// Whether the file should be deleted on dealloc. | |||
| public let deleteOnClose: Bool | |||
There was a problem hiding this comment.
Can you propose this as its own PR?
|
@aciidb0mb3r Okay, updated. |
|
Okay, rebased from master so this PR now only has the atomic write changes. |
|
@swift-ci please smoke test |
|
Thanks @czechboy0 and @vlm! |
This adds a new customization point to the
FileSystemprotocol. It also provides a default implementation that forwards nonatomic writes back to the existing implementation, and throws when atomic writes are implemented.An atomic write implementation is added for LocalFileSystem.
Tests for both have been adapted.
Depends on #1625
/cc @aciidb0mb3r @vlm