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

FileHelpers: Add FileDescriptor.read(filling buffer:) #84

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
75 changes: 75 additions & 0 deletions Sources/System/FileHelpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,78 @@ extension FileDescriptor {
return result
}

/// Reads bytes at the current file offset into a buffer until the buffer is filled or until EOF is reached.
///
/// - Parameters:
/// - buffer: The region of memory to read into.
/// - Returns: The number of bytes that were read, at most `buffer.count`.
///
/// This method either reads until `buffer` is full, or throws an error if
/// only part of the buffer was filled.
///
/// The <doc://com.apple.documentation/documentation/swift/unsafemutablerawbufferpointer/3019191-count> property of `buffer`
/// determines the number of bytes that are read into the buffer.
@_alwaysEmitIntoClient
@discardableResult
public func read(
filling buffer: UnsafeMutableRawBufferPointer
) throws -> Int {
return try _read(filling: buffer).get()
}

/// Reads bytes at the given file offset into a buffer until the buffer is filled or until EOF is reached.
///
/// - Parameters:
/// - offset: The file offset where reading begins.
/// - buffer: The region of memory to read into.
/// - Returns: The number of bytes that were read, at most `buffer.count`.
///
/// This method either reads until `buffer` is full, or throws an error if
/// only part of the buffer was filled.
///
/// Unlike ``read(filling:)``, this method preserves the file descriptor's existing offset.
///
/// The <doc://com.apple.documentation/documentation/swift/unsafemutablerawbufferpointer/3019191-count> property of `buffer`
/// determines the number of bytes that are read into the buffer.
@_alwaysEmitIntoClient
@discardableResult
public func read(
fromAbsoluteOffset offset: Int64,
filling buffer: UnsafeMutableRawBufferPointer
) throws -> Int {
return try _read(fromAbsoluteOffset: offset, filling: buffer).get()
}

@usableFromInline
internal func _read(
Copy link
Member

Choose a reason for hiding this comment

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

If we want to make this function opaque, then it'll need to come with availability, which means that the read(filling:) methods will need to have it too.

@milseman Is there a good reason writeAll is implemented using an opaque function?

fromAbsoluteOffset offset: Int64? = nil,
filling buffer: UnsafeMutableRawBufferPointer
) -> Result<Int, Errno> {
var idx = 0
loop: while idx < buffer.count {
let readResult: Result<Int, Errno>
if let offset = offset {
readResult = _read(
fromAbsoluteOffset: offset + Int64(idx),
into: UnsafeMutableRawBufferPointer(rebasing: buffer[idx...]),
retryOnInterrupt: true
)
} else {
readResult = _readNoThrow(
into: UnsafeMutableRawBufferPointer(rebasing: buffer[idx...]),
retryOnInterrupt: true
)
}
switch readResult {
case .success(let numBytes) where numBytes == 0: break loop // EOF
case .success(let numBytes): idx += numBytes
case .failure(let err): return .failure(err)
}
}
assert(idx <= buffer.count)
return .success(idx)
}

/// Writes a sequence of bytes to the current offset
/// and then updates the offset.
///
Expand All @@ -46,6 +118,9 @@ extension FileDescriptor {
/// increments that position by the number of bytes written.
/// See also ``seek(offset:from:)``.
///
/// This method either writes the entire contents of `sequence`,
/// or throws an error if only part of the content was written.
///
/// If `sequence` doesn't implement
/// the <doc://com.apple.documentation/documentation/swift/sequence/3128824-withcontiguousstorageifavailable> method,
/// temporary space will be allocated as needed.
Expand Down
11 changes: 10 additions & 1 deletion Sources/System/FileOperations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,23 @@ extension FileDescriptor {
into buffer: UnsafeMutableRawBufferPointer,
retryOnInterrupt: Bool = true
) throws -> Int {
try _read(into: buffer, retryOnInterrupt: retryOnInterrupt).get()
try _readNoThrow(into: buffer, retryOnInterrupt: retryOnInterrupt).get()
}

// NOTE: This function (mistakenly marked as throws) is vestigial but remains to preserve ABI.
@usableFromInline
internal func _read(
into buffer: UnsafeMutableRawBufferPointer,
retryOnInterrupt: Bool
) throws -> Result<Int, Errno> {
_readNoThrow(into: buffer, retryOnInterrupt: retryOnInterrupt)
}

@usableFromInline
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs availability, right @lorentey ?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately yes, and on ABI stable platforms, read will need to dispatch to either _read or _readNoThrow, depending on the platform & version we're running on.

Given the importance of this particular entry point, I think it's worth considering marking the new non-throwing variant @_alwaysEmitIntoClient @inline(never) to prevent the availability check.

Otherwise we'll need to mess with #if SWIFT_PACKAGE to enable/disable availability checking based on whether or not we're building the ABI stable variant.

I think it's okay to land this without resolving the availability mess -- ABI stable builds aren't possible to do within this repo, and I wouldn't want to force Simon to have to deal with a tricky problem based on partial info and no tooling. It's tricky enough to get right when we have the benefit of the ABI checker and a working build system. (Unless he really really wants to do it, of course!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't want to force Simon to have to deal with a tricky problem based on partial info and no tooling. It's tricky enough to get right when we have the benefit of the ABI checker and a working build system. (Unless he really really wants to do it, of course!)

😅 sounds like I might be opening a can of worms here. It sounds like merging this PR without addressing the ABI issue might be the quickest solution. After merging though, I'm happy to collaborate on how to please the ABI checker, but I confess this isn't something I've done before.

internal func _readNoThrow(
into buffer: UnsafeMutableRawBufferPointer,
retryOnInterrupt: Bool
) -> Result<Int, Errno> {
valueOrErrno(retryOnInterrupt: retryOnInterrupt) {
system_read(self.rawValue, buffer.baseAddress, buffer.count)
}
Expand Down
61 changes: 60 additions & 1 deletion Tests/SystemTests/FileOperationsTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -160,5 +160,64 @@ final class FileOperationsTest: XCTestCase {
issue26.runAllTests()

}
}

func testReadFillingFromFileEOF() async throws {
// Create a temporary file.
let tmpPath = "/tmp/\(UUID().uuidString)"
defer {
unlink(tmpPath)
}

// Write some bytes.
var abc = "abc"
let byteCount = abc.count
let writeFd = try FileDescriptor.open(tmpPath, .readWrite, options: [.create, .truncate], permissions: .ownerReadWrite)
try writeFd.closeAfter {
try abc.withUTF8 {
XCTAssertEqual(try writeFd.writeAll(UnsafeRawBufferPointer($0)), byteCount)
}
}

// Try and read more bytes than were written.
let readFd = try FileDescriptor.open(tmpPath, .readOnly)
try readFd.closeAfter {
let readBytes = try Array<UInt8>(unsafeUninitializedCapacity: byteCount + 1) { buf, count in
count = try readFd.read(filling: UnsafeMutableRawBufferPointer(buf))
XCTAssertEqual(count, byteCount)
}
XCTAssertEqual(readBytes, Array(abc.utf8))
}
}

/// This `#if` is present because, While `read(filling:)` is available on all platforms, this test
/// makes use of `FileDescriptor.pipe()` which is not available on Windows.
#if !os(Windows)
func testReadFillingFromPipe() async throws {
let pipe = try FileDescriptor.pipe()
defer {
try? pipe.writeEnd.close()
try? pipe.readEnd.close()
}
var abc = "abc"
var def = "def"
let abcdef = abc + def

try abc.withUTF8 {
XCTAssertEqual(try pipe.writeEnd.writeAll(UnsafeRawBufferPointer($0)), 3)
}

async let readBytes = try Array<UInt8>(unsafeUninitializedCapacity: abcdef.count) { buf, count in
count = try pipe.readEnd.read(filling: UnsafeMutableRawBufferPointer(buf))
XCTAssertEqual(count, abcdef.count)
}

try def.withUTF8 {
XCTAssertEqual(try pipe.writeEnd.writeAll(UnsafeRawBufferPointer($0)), 3)
}

let _readBytes = try await readBytes

XCTAssertEqual(_readBytes, Array(abcdef.utf8))
}
#endif
}