Skip to content

SR-13822: FileHandle closeOnDealloc is not respected #2932

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

Merged
merged 1 commit into from
Dec 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 17 additions & 14 deletions Sources/Foundation/FileHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ open class FileHandle : NSObject {
// - deinit tries to .sync { … } to serialize the work on the handle queue, _which we're already on_
// - deadlock! DispatchQueue's deadlock detection triggers and crashes us.
// since all operations on the handle queue retain the handle during use, if the handle is being deinited, then there are no more operations on the queue, so this is serial with respect to them anyway. Just close the handle immediately.
try? _immediatelyClose()
try? _immediatelyClose(closeFd: _closeOnDealloc)
}

// MARK: -
Expand Down Expand Up @@ -619,8 +619,9 @@ open class FileHandle : NSObject {
try _immediatelyClose()
}
}

private func _immediatelyClose() throws {

// Shutdown any read/write sources and handlers, and optionally close the file descriptor.
private func _immediatelyClose(closeFd: Bool = true) throws {
guard self != FileHandle._nulldeviceFileHandle else { return }
guard _isPlatformHandleValid else { return }

Expand All @@ -632,18 +633,20 @@ open class FileHandle : NSObject {
writabilitySource = nil
readabilitySource = nil
privateAsyncVariablesLock.unlock()

#if os(Windows)
guard CloseHandle(self._handle) else {
throw _NSErrorWithWindowsError(GetLastError(), reading: true)
}
self._handle = INVALID_HANDLE_VALUE
#else
guard _close(_fd) >= 0 else {
throw _NSErrorWithErrno(errno, reading: true)

if closeFd {
#if os(Windows)
guard CloseHandle(self._handle) else {
throw _NSErrorWithWindowsError(GetLastError(), reading: true)
}
self._handle = INVALID_HANDLE_VALUE
#else
guard _close(_fd) >= 0 else {
throw _NSErrorWithErrno(errno, reading: true)
}
_fd = -1
#endif
}
_fd = -1
#endif
}

// MARK: -
Expand Down
36 changes: 35 additions & 1 deletion Tests/Foundation/Tests/TestFileHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,40 @@ class TestFileHandle : XCTestCase {
XCTAssertTrue(notificationReceived, "Notification should be sent")
}
}


#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT
func test_closeOnDealloc() throws {
try withTemporaryDirectory() { (url, path) in
let data = try XCTUnwrap("hello".data(using: .utf8))

// closeOnDealloc: true, 2nd write should throw.
do {
let fileUrl = url.appendingPathComponent("testfile")
let fd = try FileHandle._openFileDescriptorForURL(fileUrl, flags: O_CREAT | O_RDWR, reading: false)
do {
let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
XCTAssertNoThrow(try fh.write(contentsOf: data))
}
let fh2 = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
XCTAssertThrowsError(try fh2.write(contentsOf: data))
}

// closeOnDealloc: false, 2nd write should succeed.
do {
let fileUrl = url.appendingPathComponent("testfile2")
let fd = try FileHandle._openFileDescriptorForURL(fileUrl, flags: O_CREAT | O_RDWR, reading: false)
do {
let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: false)
XCTAssertNoThrow(try fh.write(contentsOf: data))
}
// Close the file handle after this write, dont leave it open after this test.
let fh2 = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
XCTAssertNoThrow(try fh2.write(contentsOf: data))
}
}
}
#endif

static var allTests : [(String, (TestFileHandle) -> () throws -> ())] {
var tests: [(String, (TestFileHandle) -> () throws -> ())] = [
("testReadUpToCount", testReadUpToCount),
Expand Down Expand Up @@ -607,6 +640,7 @@ class TestFileHandle : XCTestCase {
("test_nullDevice", test_nullDevice),
("testHandleCreationAndCleanup", testHandleCreationAndCleanup),
("testOffset", testOffset),
("test_closeOnDealloc", test_closeOnDealloc),
])
#endif

Expand Down