-
-
Notifications
You must be signed in to change notification settings - Fork 6
Split into two ranges to read messages if offsetInFileOfOldestMessage > offsetInFileAtEndOfNewestMessage to avoid possible loop #72
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
Changes from all commits
b267dbd
e233c94
78f3cb5
b9c8ff7
f26e921
b3d64ec
54e61e2
f623856
46de4c5
57b3304
27c9f65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -47,9 +47,7 @@ public final class CacheAdvance<T: Codable> { | |||||
| self.init( | ||||||
| fileURL: fileURL, | ||||||
| writer: try FileHandle(forWritingTo: fileURL), | ||||||
| reader: try CacheReader( | ||||||
| forReadingFrom: fileURL, | ||||||
| maximumBytes: maximumBytes), | ||||||
| reader: try CacheReader(forReadingFrom: fileURL), | ||||||
| header: try CacheHeaderHandle( | ||||||
| forReadingFrom: fileURL, | ||||||
| maximumBytes: maximumBytes, | ||||||
|
|
@@ -172,10 +170,35 @@ public final class CacheAdvance<T: Codable> { | |||||
| try header.checkFile() | ||||||
|
|
||||||
| var messages = [T]() | ||||||
| while let encodedMessage = try reader.nextEncodedMessage() { | ||||||
| messages.append(try decoder.decode(T.self, from: encodedMessage)) | ||||||
| } | ||||||
| if reader.offsetInFileOfOldestMessage < reader.offsetInFileAtEndOfNewestMessage { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the cache is empty I assume that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would also be nice to validate my assumption that
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Per this comment block CacheAdvance/Tests/CacheAdvanceTests/CacheAdvanceTests.swift Lines 367 to 368 in 3a1ed6f
when the reading handle and the writing handle point at the same position the file is empty. The reader starts out at
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. like this idea too. handle |
||||||
| // There is only one range: | `offsetInFileOfOldestMessage` -> `offsetInFileAtEndOfNewestMessage` | | ||||||
| let encodedMessages = try reader.encodedMessagesFromOffset( | ||||||
| reader.offsetInFileOfOldestMessage, | ||||||
| endOffset: reader.offsetInFileAtEndOfNewestMessage) | ||||||
| for encodedMessage in encodedMessages { | ||||||
| messages.append(try decoder.decode(T.self, from: encodedMessage)) | ||||||
| } | ||||||
| } else if reader.offsetInFileOfOldestMessage == reader.offsetInFileAtEndOfNewestMessage { | ||||||
| // This is an empty cache. | ||||||
| return [] | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||
| } else { | ||||||
| // In this case, the messages could be split to two ranges | ||||||
| // | First Range | (GAP: ignore) | Second Range | | ||||||
|
bachand marked this conversation as resolved.
|
||||||
|
|
||||||
| // This is second range: | `offsetInFileOfOldestMessage` -> EOF | | ||||||
| let olderMessages = try reader.encodedMessagesFromOffset(reader.offsetInFileOfOldestMessage) | ||||||
| for encodedMessage in olderMessages { | ||||||
| messages.append(try decoder.decode(T.self, from: encodedMessage)) | ||||||
| } | ||||||
|
|
||||||
| // This is first range: | `expectedEndOfHeaderInFile` -> `offsetInFileAtEndOfNewestMessage` | | ||||||
| let newerMessages = try reader.encodedMessagesFromOffset( | ||||||
| FileHeader.expectedEndOfHeaderInFile, | ||||||
| endOffset: reader.offsetInFileAtEndOfNewestMessage) | ||||||
| for encodedMessage in newerMessages { | ||||||
| messages.append(try decoder.decode(T.self, from: encodedMessage)) | ||||||
| } | ||||||
| } | ||||||
| // Now that we've read all messages, seek back to the oldest message. | ||||||
| try reader.seekToBeginningOfOldestMessage() | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,12 +23,9 @@ final class CacheReader { | |
|
|
||
| /// Creates a new instance of the receiver. | ||
| /// | ||
| /// - Parameters: | ||
| /// - file: The file URL indicating the desired location of the on-disk store. This file should already exist. | ||
| /// - maximumBytes: The maximum size of the cache, in bytes. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 |
||
| init(forReadingFrom file: URL, maximumBytes: Bytes) throws { | ||
| /// - Parameter file: The file URL indicating the desired location of the on-disk store. This file should already exist. | ||
| init(forReadingFrom file: URL) throws { | ||
| reader = try FileHandle(forReadingFrom: file) | ||
| self.maximumBytes = maximumBytes | ||
| } | ||
|
|
||
| deinit { | ||
|
|
@@ -44,51 +41,27 @@ final class CacheReader { | |
| reader.offsetInFile | ||
| } | ||
|
|
||
| /// Returns the next encodable message, seeking to the beginning of the next message. | ||
| func nextEncodedMessage() throws -> Data? { | ||
| let startingOffset = offsetInFile | ||
|
|
||
| guard startingOffset != offsetInFileAtEndOfNewestMessage else { | ||
| // We're at the last message. | ||
| return nil | ||
| } | ||
|
|
||
| switch try nextEncodedMessageSpan() { | ||
| case let .span(messageLength): | ||
| // Check our assumptions before we try to read the message. | ||
| let endOfMessage = startingOffset + UInt64(MessageSpan.storageLength) + UInt64(messageLength) | ||
| let startingOffsetIsBeforeEndOfNewestMessageAndDoesNotExceedEndOfNewestMessage = startingOffset < offsetInFileAtEndOfNewestMessage && endOfMessage <= offsetInFileAtEndOfNewestMessage | ||
| let startingOffsetIsAfterEndOfNewestMessageAndDoesNotExceedEndOfFile = offsetInFileAtEndOfNewestMessage < startingOffset && endOfMessage <= maximumBytes | ||
| guard | ||
| startingOffsetIsBeforeEndOfNewestMessageAndDoesNotExceedEndOfNewestMessage | ||
| || startingOffsetIsAfterEndOfNewestMessageAndDoesNotExceedEndOfFile | ||
| else { | ||
| // The offsetInFileAtEndOfNewestMessage is incorrect. This likely occured due to a crash when writing our header file. | ||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
|
dfed marked this conversation as resolved.
|
||
|
|
||
| let message = try reader.readDataUp(toLength: Int(messageLength)) | ||
| guard message.count > 0 else { | ||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
|
|
||
| return message | ||
|
|
||
| case .emptyRead: | ||
| guard offsetInFileAtEndOfNewestMessage < startingOffset else { | ||
| // We started reading before the offset of the end of the newest message, therefore we expected a message to be read. We instead read an empty space, meaning that the file is corrupt. | ||
| throw CacheAdvanceError.fileCorrupted | ||
| /// Returns the encodable messages in a range | ||
| /// | ||
| /// - Parameter startOffset: the offset from which to start reading | ||
| /// - Parameter endOffset: the offset at which to stop reading. If `nil`, the end offset will be the EOF | ||
| func encodedMessagesFromOffset(_ startOffset: UInt64, endOffset: UInt64? = nil) throws -> [Data] { | ||
|
dfed marked this conversation as resolved.
|
||
| var encodedMessages = [Data]() | ||
| try reader.seek(to: startOffset) | ||
| while let data = try nextEncodedMessage() { | ||
| encodedMessages.append(data) | ||
| if let endOffset = endOffset { | ||
| if offsetInFile == endOffset { | ||
| break | ||
| } else if offsetInFile > endOffset { | ||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
| } | ||
|
|
||
| // We know the next message is at the end of the file header. Let's seek to it. | ||
| try reader.seek(to: FileHeader.expectedEndOfHeaderInFile) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the library is safer against unknown unknown errors without this line of code. |
||
|
|
||
| // We know there's a message to read now that we're at the start of the file. | ||
| return try nextEncodedMessage() | ||
|
|
||
| case .invalidFormat: | ||
| } | ||
| if let endOffset = endOffset, offsetInFile != endOffset { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This make no difference compared to the code before this. However, this looks more readable to handle
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh. This code is catching the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh, I misunderstood your question. The
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree it would be nice to make this more clear.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll take this in a follow-up PR |
||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
| return encodedMessages | ||
| } | ||
|
|
||
| /// Seeks to the beginning of the oldest message in the file. | ||
|
|
@@ -114,6 +87,26 @@ final class CacheReader { | |
|
|
||
| // MARK: Private | ||
|
|
||
| /// Returns the next encodable message, seeking to the beginning of the next message. | ||
| private func nextEncodedMessage() throws -> Data? { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mark this func as private since it is not used by other files any more. |
||
| switch try nextEncodedMessageSpan() { | ||
| case let .span(messageLength): | ||
| let message = try reader.readDataUp(toLength: Int(messageLength)) | ||
| guard message.count > 0 else { | ||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
|
|
||
| return message | ||
|
|
||
| case .emptyRead: | ||
| // An empty read means we hit the EOF. It is the responsibility of the calling code to validate this assumption. | ||
| return nil | ||
|
|
||
| case .invalidFormat: | ||
| throw CacheAdvanceError.fileCorrupted | ||
| } | ||
| } | ||
|
|
||
| /// Returns the next encoded message span, seeking to the end the span. | ||
| private func nextEncodedMessageSpan() throws -> NextMessageSpan { | ||
| let messageSizeData = try reader.readDataUp(toLength: MessageSpan.storageLength) | ||
|
|
@@ -132,7 +125,6 @@ final class CacheReader { | |
| } | ||
|
|
||
| private let reader: FileHandle | ||
| private let maximumBytes: Bytes | ||
|
|
||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.