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.

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

SR-13822: FileHandle closeOnDealloc is not respected #2932

Merged
merged 1 commit into from
Dec 13, 2020
Merged
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
SR-13822: FileHandle closeOnDealloc is not respected
spevans committed Dec 4, 2020
commit af1c2c7a95af8bb8023a64d181c75a936bf4e53d
31 changes: 17 additions & 14 deletions Sources/Foundation/FileHandle.swift
Original file line number Diff line number Diff line change
@@ -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: -
@@ -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 }

@@ -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: -
36 changes: 35 additions & 1 deletion Tests/Foundation/Tests/TestFileHandle.swift
Original file line number Diff line number Diff line change
@@ -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),
@@ -607,6 +640,7 @@ class TestFileHandle : XCTestCase {
("test_nullDevice", test_nullDevice),
("testHandleCreationAndCleanup", testHandleCreationAndCleanup),
("testOffset", testOffset),
("test_closeOnDealloc", test_closeOnDealloc),
])
#endif