-
-
Notifications
You must be signed in to change notification settings - Fork 368
refactor: Migrate SentryMsgPackSerializer from Objective-C to Swift #6143
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
Open
philprime
wants to merge
15
commits into
main
Choose a base branch
from
philprime/msg-pack-serializer-null-handling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
a121c88
refactor: Add nullability-handling to SentryMsgPackSerializer with co…
philprime 6eb1e43
Merge remote-tracking branch 'origin/main' into philprime/msg-pack-se…
philprime 3021956
feat: Enhance SentryMsgPackSerializer with error handling and additio…
philprime 810f132
refactor: Remove SentryMsgPackSerializer and migrate to Swift impleme…
philprime b73f131
fix: Update keyData handling in SentryMsgPackSerializer to use try fo…
philprime 8d45ba3
feat: Add TestStreamableObject for enhanced serialization testing
philprime 9b91dac
Merge remote-tracking branch 'origin/main' into philprime/msg-pack-se…
philprime 7726ae1
fix potential overflow for very large files
philprime 2307fe1
Merge remote-tracking branch 'origin/main' into philprime/msg-pack-se…
philprime 5181e9e
remove legacy extensions
philprime 568d287
resolve conversion mistakes
philprime 157de83
Merge remote-tracking branch 'origin/main' into philprime/msg-pack-se…
philprime e7abc0a
Fix xcode proj
philprime 0442fd3
Refactor SentryMsgPackSerializer to improve file serialization and er…
philprime e6ec86d
Merge branch 'main' into philprime/msg-pack-serializer-null-handling
philprime File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| @testable import Sentry | ||
|
|
||
| private class ErrorInputStream: InputStream { | ||
| override var hasBytesAvailable: Bool { | ||
| return true | ||
| } | ||
|
|
||
| override func read(_ buffer: UnsafeMutablePointer<UInt8>, maxLength len: Int) -> Int { | ||
| return -1 // Simulate read error | ||
| } | ||
|
|
||
| override func open() { | ||
| // No-op | ||
| } | ||
|
|
||
| override func close() { | ||
| // No-op | ||
| } | ||
| } | ||
|
|
||
| public class TestStreamableObject: NSObject, SentryStreamable { | ||
|
|
||
| private let shouldReturnNilInputStream: Bool | ||
| private let streamSizeValue: UInt? | ||
| private let shouldReturnErrorStream: Bool | ||
|
|
||
| public init(streamSize: UInt?, shouldReturnNilInputStream: Bool, shouldReturnErrorStream: Bool = false) { | ||
| self.streamSizeValue = streamSize | ||
| self.shouldReturnNilInputStream = shouldReturnNilInputStream | ||
| self.shouldReturnErrorStream = shouldReturnErrorStream | ||
| super.init() | ||
| } | ||
|
|
||
| public func asInputStream() -> InputStream? { | ||
| if shouldReturnNilInputStream { | ||
| return nil | ||
| } | ||
| if shouldReturnErrorStream { | ||
| return ErrorInputStream() | ||
| } | ||
| return InputStream(data: Data()) | ||
| } | ||
|
|
||
| public func streamSize() -> UInt? { | ||
| return streamSizeValue | ||
| } | ||
|
|
||
| // MARK: - Convenience factory methods for common test scenarios | ||
|
|
||
| public static func objectWithNilInputStream() -> TestStreamableObject { | ||
| return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: true) | ||
| } | ||
|
|
||
| public static func objectWithZeroSize() -> TestStreamableObject { | ||
| return TestStreamableObject(streamSize: 0, shouldReturnNilInputStream: false) | ||
| } | ||
|
|
||
| public static func objectWithNegativeSize() -> TestStreamableObject { | ||
| return TestStreamableObject(streamSize: nil, shouldReturnNilInputStream: false) | ||
| } | ||
|
|
||
| public static func objectWithErrorStream() -> TestStreamableObject { | ||
| return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: false, shouldReturnErrorStream: true) | ||
| } | ||
|
|
||
| public static func objectWithZeroBytesRead() -> TestStreamableObject { | ||
| return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: false, shouldReturnErrorStream: false) | ||
| } | ||
|
|
||
| public static func objectWithLargeSize() -> TestStreamableObject { | ||
| // Return size larger than UInt32.max to test truncation | ||
| return TestStreamableObject( | ||
| streamSize: UInt.max, | ||
| shouldReturnNilInputStream: false, | ||
| shouldReturnErrorStream: false | ||
| ) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| extension Data: SentryStreamable { | ||
| func asInputStream() -> InputStream? { | ||
| return InputStream(data: self) | ||
| } | ||
|
|
||
| func streamSize() -> UInt? { | ||
| return UInt(self.count) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| /** | ||
| * This is a partial implementation of the MessagePack format. | ||
| * We only need to concatenate a list of NSData into an envelope item. | ||
| */ | ||
| class SentryMsgPackSerializer { | ||
| @objc | ||
| static func serializeDictionary(toMessagePack dictionary: [String: Any], intoFile fileURL: URL) -> Bool { | ||
| do { | ||
| let data = try serializeDictionaryToMessagePack(dictionary) | ||
| try data.write(to: fileURL) | ||
| return true | ||
| } catch { | ||
| SentrySDKLog.error("Failed to serialize dictionary to MessagePack or write to file - Error: \(error)") | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| static func serializeDictionaryToMessagePack(_ dictionary: [String: Any]) throws -> Data { // swiftlint:disable:this function_body_length | ||
| let outputStream = OutputStream.toMemory() | ||
| outputStream.open() | ||
| defer { outputStream.close() } | ||
|
|
||
| let mapHeader = UInt8(0x80 | dictionary.count) // Map up to 15 elements | ||
| _ = outputStream.write([mapHeader], maxLength: 1) | ||
|
|
||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for (key, anyValue) in dictionary { | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| guard let value = anyValue as? SentryStreamable else { | ||
| throw SentryMsgPackSerializerError.invalidValue("Value does not conform to SentryStreamable: \(anyValue)") | ||
| } | ||
| guard let keyData = key.data(using: .utf8) else { | ||
| throw SentryMsgPackSerializerError.invalidInput("Could not encode key as UTF-8: \(key)") | ||
| } | ||
|
|
||
| let str8Header: UInt8 = 0xD9 // String up to 255 characters | ||
| let keyLength = UInt8(truncatingIfNeeded: keyData.count) // Truncates if > 255, matching Objective-C behavior | ||
| _ = outputStream.write([str8Header], maxLength: 1) | ||
| _ = outputStream.write([keyLength], maxLength: 1) | ||
|
|
||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| try keyData.withUnsafeBytes { bytes in | ||
| guard let bufferAddress = bytes.bindMemory(to: UInt8.self).baseAddress else { | ||
| throw SentryMsgPackSerializerError.invalidInput("Could not get buffer address for key: \(key)") | ||
| } | ||
| _ = outputStream.write(bufferAddress, maxLength: keyData.count) | ||
| } | ||
|
|
||
| guard let dataLength = value.streamSize(), dataLength > 0 else { | ||
| // MsgPack is being used strictly for session replay. | ||
philprime marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // An item with a length of 0 will not be useful. | ||
| // If we plan to use MsgPack for something else, | ||
| // this needs to be re-evaluated. | ||
| SentrySDKLog.error("Data for MessagePack dictionary has no content - Input: \(value)") | ||
philprime marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| throw SentryMsgPackSerializerError.emptyData("Empty data for MessagePack dictionary") | ||
| } | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| let valueLength = UInt32(truncatingIfNeeded: dataLength) | ||
| // We will always use the 4 bytes data length for simplicity. | ||
| // Worst case we're losing 3 bytes. | ||
| let bin32Header: UInt8 = 0xC6 | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| _ = outputStream.write([bin32Header], maxLength: 1) | ||
|
|
||
| // Write UInt32 as big endian bytes | ||
| let lengthBytes = [ | ||
| UInt8((valueLength >> 24) & 0xFF), | ||
| UInt8((valueLength >> 16) & 0xFF), | ||
| UInt8((valueLength >> 8) & 0xFF), | ||
| UInt8(valueLength & 0xFF) | ||
| ] | ||
| _ = outputStream.write(lengthBytes, maxLength: 4) | ||
|
|
||
| guard let inputStream = value.asInputStream() else { | ||
| SentrySDKLog.error("Could not get input stream - Input: \(value)") | ||
| throw SentryMsgPackSerializerError.streamError("Could not get input stream from value") | ||
| } | ||
|
|
||
| inputStream.open() | ||
| defer { inputStream.close() } | ||
|
|
||
| var buffer = [UInt8](repeating: 0, count: 1_024) | ||
| var bytesRead: Int | ||
|
|
||
| while inputStream.hasBytesAvailable { | ||
| bytesRead = inputStream.read(&buffer, maxLength: buffer.count) | ||
| if bytesRead > 0 { | ||
| _ = outputStream.write(buffer, maxLength: bytesRead) | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } else if bytesRead < 0 { | ||
| SentrySDKLog.error("Error reading bytes from input stream - Input: \(value) - \(bytesRead)") | ||
philprime marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| throw SentryMsgPackSerializerError.streamError("Error reading bytes from input stream") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| guard let data = outputStream.property(forKey: .dataWrittenToMemoryStreamKey) as? Data else { | ||
| throw SentryMsgPackSerializerError.outputError("Could not retrieve data from memory stream") | ||
| } | ||
|
|
||
| return data | ||
| } | ||
| } | ||
8 changes: 8 additions & 0 deletions
8
Sources/Swift/Tools/MsgPack/SentryMsgPackSerializerError.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| enum SentryMsgPackSerializerError: Error { | ||
| case dictionaryTooLarge | ||
| case invalidValue(String) | ||
| case invalidInput(String) | ||
| case emptyData(String) | ||
| case streamError(String) | ||
| case outputError(String) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| protocol SentryStreamable { | ||
| func asInputStream() -> InputStream? | ||
| func streamSize() -> UInt? | ||
| } | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| extension URL: SentryStreamable { | ||
| func asInputStream() -> InputStream? { | ||
| return InputStream(url: self) | ||
| } | ||
|
|
||
| func streamSize() -> UInt? { | ||
| let attributes: [FileAttributeKey: Any] | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| do { | ||
| attributes = try FileManager.default.attributesOfItem(atPath: path) | ||
| } catch { | ||
| SentrySDKLog.error("Could not read file attributes - File: \(self) - Error: \(error)") | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return nil | ||
| } | ||
| guard let fileSize = attributes[.size] as? NSNumber else { | ||
| SentrySDKLog.error("Could not read file size attribute - File: \(self)") | ||
| return nil | ||
| } | ||
| return fileSize.uintValue | ||
| } | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Serializer Memory Leak & Map Header Crash
The
SentryMsgPackSerializeraccumulates all data in memory before writing to a file, potentially causing high memory usage or OOM errors for large datasets. Also, the map header's dictionary count calculation may crash at runtime if the count exceeds 255, unlike the original Objective-C truncation behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is true? At least the first part about accumulating everything to memory. Any reason we can't use the ObjC behavior here of having the output stream write to a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double-check why I changed it, it's been a while