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 3 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
70 changes: 70 additions & 0 deletions Sources/System/FileHelpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,76 @@ extension FileDescriptor {
}
}

/// Reads bytes at the current file offset into a buffer until the buffer is filled.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the file runs out of bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I guess that's up for grabs. When you added this comment it would block. But this doesn't seem right. I added an update which checks for EOF and returns (which makes returning an Int more relevant again).

There's another option to consider. We could have read(filling:) throw an error if it encounters EOF before the buffer is filled. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I've used libraries where the fill-this-buffer-by-reading-bytes operation would signal an error if it runs out of bytes and I used ones where it would return a partial buffer. Generally I found the erroring variant more convenient, as in the other case I'd usually just end up having to signal an error myself. (This is supposing that the regular partial read is available as a separate operation, like is the case with System.) So from a usability standpoint I'd prefer this threw an error on EOF.

Unfortunately, there is no errno value that represents the EOF condition -- the original i/o interfaces do not treat this as an error. So we'll have to define a custom Error type if we want to do this.

(We'll need to step carefully here, as I'd prefer if we did not start a slippery slope where we end up with a full blown File abstraction within System -- this library ought to simply sanitize system interfaces, not reinvent them.)

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 returning the actual number of bytes read is better than throwing. How do FIFOs, etc. work in this respect?

Upon successful completion, read(), readv(), pread(), and preadv() return
the number of bytes actually read and placed in the buffer. The system
guarantees to read the number of bytes requested if the descriptor
references a normal file that has that many bytes left before the end-of-
file, but in no other case.

RETURN VALUES
If successful, the number of bytes actually read is returned. Upon reading
end-of-file, zero is returned. Otherwise, a -1 is returned and the global
variable errno is set to indicate the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current state of this PR is that it will return an Int for the number of bytes read, which will be at most buffer.count. If there were enough bytes in the file to fill the buffer then it does fill the buffer and returns buffer.count, if EOF is encountered then it will not throw, but return the number of bytes read into the buffer, which will be less than buffer.count.

How do we feel about that? LMK if you want something different.

///
/// - Parameters:
/// - buffer: The region of memory to read into.
/// - Returns: The number of bytes that were read, equal to `buffer.count`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's always equal, then we'd return a Bool. But, if we might only partly fill for an oversized buffer, then it makes sense to return the count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. If we go the route of returning a Bool are we happy to accept the asymmetry it will create with the writeAll(_:) helpers, which currently return Int even though the docs for writeAll(_:) state:

  /// Writes a sequence of bytes to the current offset
  /// and then updates the offset.
  ///
  /// - Parameter sequence: The bytes to write.
  /// - Returns: The number of bytes written, equal to the number of elements in `sequence`.
  ///
  /// This method either writes the entire contents of `sequence`,
  /// or throws an error if only part of the content was written.
...
  @_alwaysEmitIntoClient
  @discardableResult
  public func writeAll<S: Sequence>(
...

Copy link
Member

Choose a reason for hiding this comment

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

Beyond consistency, returning an integer has the very slight convenience benefit that I can write c += read(filling: buffer) to keep a running total of bytes read. (read(filling: buffer); c += buffer.count works but it is more error prone)

Returning a boolean instead of throwing an error on EOF is an option to consider. It's sort of consistent with the original interfaces, but it has the same convenience issue as returning a partial buffer -- callers would need to manually check for it and in the partial case they will typically end up throwing anyway.

So I'm slightly preferring having this return buffer.count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current state in PR returns an Int, discussed in more detail in #84 (comment).

///
/// 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 current 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.
///
/// 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 given offset.
///
/// - Parameters:
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
}