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

ReadableFileHandleProtocol.readToEnd can fail to read the complete contents of file sizes less than the single shot read limit #2769

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
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
Prev Previous commit
Rewrite ChunkRange to use either a specified offset or the current of…
…fset
rpecka committed Jul 6, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit ecbb478d63c49c1e589946dab47c5c0da7a0a5df
19 changes: 10 additions & 9 deletions Sources/NIOFileSystem/FileChunks.swift
Original file line number Diff line number Diff line change
@@ -22,8 +22,10 @@ import NIOPosix
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
public struct FileChunks: AsyncSequence {
enum ChunkRange {
case entireFile
case partial(Range<Int64>)
/// Read from the current file access offset. Useful for reading from unseekable files.
case current
/// Read from a specific offset.
case specified(Range<Int64>)
}

public typealias Element = ByteBuffer
@@ -39,13 +41,12 @@ public struct FileChunks: AsyncSequence {
internal init(
handle: SystemFileHandle,
chunkLength: ByteCount,
range: Range<Int64>
range: Range<Int64>?
) {
let chunkRange: ChunkRange
if range.lowerBound == 0, range.upperBound == .max {
chunkRange = .entireFile
let chunkRange: ChunkRange = if let range {
.specified(range)
} else {
chunkRange = .partial(range)
.current
}

// TODO: choose reasonable watermarks; this should likely be at least somewhat dependent
@@ -96,9 +97,9 @@ extension BufferedStream where Element == ByteBuffer {
) -> BufferedStream<ByteBuffer> {
let state: ProducerState
switch range {
case .entireFile:
case .current:
state = ProducerState(handle: handle, range: nil)
case .partial(let partialRange):
case .specified(let partialRange):
state = ProducerState(handle: handle, range: partialRange)
}
let protectedState = NIOLockedValueBox(state)
4 changes: 2 additions & 2 deletions Sources/NIOFileSystem/FileHandle.swift
Original file line number Diff line number Diff line change
@@ -190,7 +190,7 @@ public struct ReadFileHandle: ReadableFileHandleProtocol, _HasFileHandle {
)
}

public func readChunks(in range: Range<Int64>, chunkLength: ByteCount) -> FileChunks {
public func readChunks(in range: Range<Int64>?, chunkLength: ByteCount) -> FileChunks {
self.fileHandle.systemFileHandle.readChunks(in: range, chunkLength: chunkLength)
}

@@ -265,7 +265,7 @@ public struct ReadWriteFileHandle: ReadableAndWritableFileHandleProtocol, _HasFi
)
}

public func readChunks(in offset: Range<Int64>, chunkLength: ByteCount) -> FileChunks {
public func readChunks(in offset: Range<Int64>?, chunkLength: ByteCount) -> FileChunks {
self.fileHandle.systemFileHandle.readChunks(in: offset, chunkLength: chunkLength)
}

10 changes: 4 additions & 6 deletions Sources/NIOFileSystem/FileHandleProtocol.swift
Original file line number Diff line number Diff line change
@@ -201,7 +201,7 @@ public protocol ReadableFileHandleProtocol: FileHandleProtocol {
/// - range: The absolute offsets into the file to read.
/// - chunkLength: The maximum length of the chunk to read as a ``ByteCount``.
/// - Returns: A sequence of chunks read from the file.
func readChunks(in range: Range<Int64>, chunkLength: ByteCount) -> FileChunks
func readChunks(in range: Range<Int64>?, chunkLength: ByteCount) -> FileChunks
}

// MARK: - Read chunks with default chunk length
@@ -227,15 +227,13 @@ extension ReadableFileHandleProtocol {
///
/// - Parameters:
/// - range: A range of offsets in the file to read.
/// - chunkLength: The length of chunks to read, defaults to 128 KiB.
/// - as: Type of chunk to read.
/// - SeeAlso: ``ReadableFileHandleProtocol/readChunks(in:chunkLength:)-2dz6``
/// - Returns: An `AsyncSequence` of chunks read from the file.
public func readChunks(
in range: Range<Int64>,
chunkLength: ByteCount = .kibibytes(128)
in range: Range<Int64>?
) -> FileChunks {
return self.readChunks(in: range, chunkLength: chunkLength)
return self.readChunks(in: range, chunkLength: .kibibytes(128))
}

/// Returns an asynchronous sequence of chunks read from the file.
@@ -413,7 +411,7 @@ extension ReadableFileHandleProtocol {
var accumulator = ByteBuffer()
accumulator.reserveCapacity(readSize)

for try await chunk in self.readChunks(in: ..., chunkLength: .mebibytes(8)) {
for try await chunk in self.readChunks(in: nil, chunkLength: .mebibytes(8)) {
accumulator.writeImmutableBuffer(chunk)
if accumulator.readableBytes > maximumSizeAllowed.bytes {
throw FileSystemError(
2 changes: 1 addition & 1 deletion Sources/NIOFileSystem/Internal/SystemFileHandle.swift
Original file line number Diff line number Diff line change
@@ -1069,7 +1069,7 @@ extension SystemFileHandle: ReadableFileHandleProtocol {
}

public func readChunks(
in range: Range<Int64>,
in range: Range<Int64>?,
chunkLength size: ByteCount
) -> FileChunks {
return FileChunks(handle: self, chunkLength: size, range: range)
12 changes: 8 additions & 4 deletions Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift
Original file line number Diff line number Diff line change
@@ -61,8 +61,11 @@ final class FileHandleTests: XCTestCase {
self.bytes.getSlice(at: Int(offset), length: Int(min(length.bytes, self.chunkSize.bytes))) ?? .init()
}

func readChunks(in range: Range<Int64>, chunkLength: ByteCount) -> FileChunks {
.init(wrapping: ChunkedByteBufferSequence(buffer: self.bytes, range: range, chunkLength: chunkLength))
func readChunks(in range: Range<Int64>?, chunkLength: ByteCount) -> FileChunks {
guard let range else {
preconditionFailure("Reading from the current offset is not implemented for MockFileHandle")
}
return .init(wrapping: ChunkedByteBufferSequence(buffer: self.bytes, range: range, chunkLength: chunkLength))
}

func info() async throws -> FileInfo {
@@ -436,10 +439,11 @@ final class FileHandleTests: XCTestCase {
try await self.withHandle(forFileAtPath: privateTempDirPath.appending("fifo"), accessMode: .readWrite) {
handle in
let someBytes = ByteBuffer(repeating: 42, count: 1546)

try await handle.write(contentsOf: someBytes.readableBytesView, toAbsoluteOffset: 0)

let readSomeBytes = try await handle.readToEnd(maximumSizeAllowed: .bytes(1546))
XCTAssertEqual(readSomeBytes, someBytes)
let contents = try await handle.readToEnd(maximumSizeAllowed: .bytes(1546))
XCTAssertEqual(contents, someBytes, "Data read back from the fifo should match what was written")
}
}