Skip to content

Add an option to not delete TemporaryFile on dealloc #1625

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

Merged
merged 2 commits into from
Jun 27, 2018
Merged

Add an option to not delete TemporaryFile on dealloc #1625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d861a17
Add an option to not delete TemporaryFile on dealloc
czechboy0 Jun 26, 2018
dc9b793
Delete the temp file at the end of the test
czechboy0 Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Sources/Basic/TemporaryFile.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

/// Creates an instance of Temporary file. The temporary file will live on disk until the instance
/// goes out of scope.
Expand All @@ -84,11 +87,13 @@ public final class TemporaryFile {
/// set, dir will be set to `/tmp/`.
/// - prefix: The prefix to the temporary file name.
/// - suffix: The suffix to the temporary file name.
/// - deleteOnClose: Whether the file should get deleted when the instance is deallocated.
///
/// - Throws: TempFileError
public init(dir: AbsolutePath? = nil, prefix: String = "TemporaryFile", suffix: String = "") throws {
public init(dir: AbsolutePath? = nil, prefix: String = "TemporaryFile", suffix: String = "", deleteOnClose: Bool = true) throws {
self.suffix = suffix
self.prefix = prefix
self.deleteOnClose = deleteOnClose
// Determine in which directory to create the temporary file.
self.dir = determineTempDirectory(dir)
// Construct path to the temporary file.
Expand All @@ -109,7 +114,9 @@ public final class TemporaryFile {

/// Remove the temporary file before deallocating.
deinit {
unlink(path.asString)
if deleteOnClose {
unlink(path.asString)
}
}
}

Expand Down
30 changes: 30 additions & 0 deletions Tests/BasicTests/TemporaryFileTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,35 @@ class TemporaryFileTests: XCTestCase {
// File should be deleted now.
XCTAssertFalse(isFile(filePath))
}

func testNoCleanupTemporaryFile() throws {
let filePath: AbsolutePath
do {
let file = try TemporaryFile(deleteOnClose: false)

// Check if file is created.
XCTAssertTrue(isFile(file.path))

// Try writing some data to the file.
let stream = BufferedOutputByteStream()
stream <<< "foo"
stream <<< "bar"
stream <<< "baz"
try fputs(stream.bytes.contents, file.fileHandle)

// Go to the beginning of the file.
file.fileHandle.seek(toFileOffset: 0)
// Read the contents.
let contents = try? file.fileHandle.readFileContents()
XCTAssertEqual(contents, "foobarbaz")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be simplified I think. We can write data in the do block and then just read it outside the block using the FileSystem .

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I kept consistent with the other tests in this file, do you think they should all be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there was a reason to do that for other tests but anyway it was just a nitpick. LGTM!


filePath = file.path
}
// File should not be deleted.
XCTAssertTrue(isFile(filePath))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also remove the temporary file before exiting this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed.

// Delete the file now
try localFileSystem.removeFileTree(filePath)
}

func testCanCreateUniqueTempFiles() throws {
let filePathOne: AbsolutePath
Expand Down Expand Up @@ -133,6 +162,7 @@ class TemporaryFileTests: XCTestCase {
static var allTests = [
("testBasicReadWrite", testBasicReadWrite),
("testCanCreateUniqueTempFiles", testCanCreateUniqueTempFiles),
("testNoCleanupTemporaryFile", testNoCleanupTemporaryFile),
("testBasicTemporaryDirectory", testBasicTemporaryDirectory),
("testCanCreateUniqueTempDirectories", testCanCreateUniqueTempDirectories),
("testLeaks", testLeaks),
Expand Down