Improvement/attachments and requests #147

melekr · 2024-12-18T00:09:25Z

This PR refactors the MultipartRequest and Attachments implementation to transition from in-memory multipart body construction to disk-based streaming using a temporary file and improve filenameExtension detection.

Why?

The previous implementation (in-memory NSMutableData) held the entire multipart body in memory, risking high memory usage and even crashes for large reports or numerous chunky attachments (which is limited by available RAM), streaming to disk allows the sdk to process larger reports and attachments reliably.
Content-Length is now calculated from the size of the temporary file, avoiding the need to hold the entire payload in memory
Modern MIME Type Detection that leverage UTType APIs for iOS 14+/tvOS/macOS 11+ with fallbacks for older platforms

Changes

Replace NSMutableData operation with streaming Data to a temporary file
Use httpBodyStream to stream file content directly into the request body
Use FileHandle to manage file writing
Calculate file size after writing to ensure accurate HTTP header
defer file handling
Migrate Attachements to modern UniformTypeIdentifiers API, keep legacy support

ref: BT-5311

- Replace NSMutableData operation with streaming Data to a temporary file - Use httpBodyStream to stream file content directly into the request body - Use FileHandle to manage file writing - Calculate file size after writing to ensure accurate HTTP header - defer file handling

- Migrate to modern UniformTypeIdentifiers API, keep legacy support - Cleanup

konraddysput

The pull request description doesn't tell me why exactly we need it. Can you please share more thoughts on this?

Examples/Example-iOS-ObjC/ViewController.m

konraddysput · 2025-01-07T09:11:33Z

Package.swift

@@ -6,7 +6,7 @@ let package = Package(
    name: "Backtrace",
    platforms: [
        .iOS(.v12),
-        .macOS(.v10_13),
+        .macOS(.v11),


why do we want to bump the macos target?

macos 11 is way past its end of life, this was a perfect opportunity to let it go.
Next bump should be :

.iOS(.v13), .macOS(.v12), .tvOS(.v13)

should it this be a part of this pull request? Later if we will try to find where have we changed the macos version, it would be hard to understand why we did that here

konraddysput · 2025-01-07T09:12:15Z

Sources/Public/BacktraceCrashReporter.swift

@@ -50,8 +50,13 @@ extension BacktraceCrashReporter: CrashReporting {
    func generateLiveReport(exception: NSException? = nil,
                            attributes: Attributes,
                            attachmentPaths: [String] = []) throws -> BacktraceReport {
+        let reportData: Data


why - generateLiveReport allows to pass nullable exception - why it matters?

for testing, will remove.

konraddysput · 2025-01-07T09:38:23Z

Sources/Features/Network/MultipartRequest.swift

-        let body = NSMutableData()
+
+        // temporary file to stream data
+        let tempURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString)


what happen to the file if the app is restarted while file is being created?

file is created in Temp directory managed by OS, if the app is restarted there is no guarantee the file is persisted but :

OOM : MultipartRequest will be retried/reconstructed from the already generated and persisted live report.

BacktraceReport/Resource : MultipartRequest will be retried/reconstructed from repository

Watcher: MultipartRequest will be retried/reconstructed from repository

unlike NSMutableData it's even possible to recover multipart request file! (not sure if necessary but can be a good excuse to consider for // TODO: T16698 - Add retry logic)

konraddysput · 2025-01-07T09:39:32Z

Sources/Features/Network/MultipartRequest.swift

+            writeToFile(fileHandle, "--\(boundary)\r\n")
+            writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attribute.key)\"\r\n\r\n")
+            writeToFile(fileHandle, "\(attribute.value)\r\n")


shouldn't we do one write operation instead of multiple? I know opening writing and closing cost resources and time

My intention was multiple small write operations with batched writes to reduce system overhead, i'll look into it.

hmm i think it would be faster if we create a string with attributes in memory and then do one write operation. There is no chance we will oom the app via string with attributes so we should be good to go

konraddysput · 2025-01-09T16:33:28Z

Sources/Features/Network/MultipartRequest.swift

-            body.appendString(boundaryPrefix)
-            body.appendString("Content-Disposition: form-data; name=\"\(attachment.filename)\"; filename=\"\(attachment.filename)\"\r\n")
-            body.appendString("Content-Type: \(attachment.mimeType)\r\n\r\n")
-            body.append(attachment.data)


can we stream data instead of reading it to memory?

we can use a Stream.

Benefits:

no longer holds attachment or report data in memory before writing since data is written directly to file handle as it's being read

Data is never loaded entirely into memory, great for multiple and/or large attachments

Caviats:

Memory and buffer management

Error handling (incomplete streams, stream cannot be opened

Small buffer chunks

comment is on original not the updated code

melekr · 2025-01-14T22:30:34Z

Holding data in memory for attributes or attachments changes the purpose of writing into files.
A hybrid approach would be buffering data in chunks, using a buffered stream.

konraddysput · 2025-01-16T12:04:56Z

Sources/Features/Error/BacktraceError.swift

+        case .fileCreationFailed(let url):
+            return "File Error occurred: \(url)."
+        case .fileWriteFailed:
+            return "File Write Error occurred."


Suggestion:

I think file is to generic and might be weird from the developer perspective that the file means a temporary file during HTTP submission. My recommendation is to make it more explicit by saying "Temporary submission file" instead.

yes, it looks out of place
also need to properly handle an 'attachmentError'.

konraddysput · 2025-01-16T12:06:45Z

Sources/Features/Network/MultipartRequest.swift

+        do {
+            let fileCreated = FileManager.default.createFile(atPath: tempURL.path, contents: nil, attributes: nil)
+            if !fileCreated {
+                throw HttpError.fileCreationFailed(tempURL)


it won't be captured by the L:111 - which means we will return a generic error rather this this one?

konraddysput · 2025-01-16T12:08:04Z

Sources/Features/Network/MultipartRequest.swift

+            try writeToFile(fileHandle, attributesString)
+
+            // report
+            var reportString = "--\(boundary)\r\n"


It would be better if we start with report rather than with attributes - if the application stops during submission part, we can still submit a valid report.

konraddysput · 2025-01-16T12:09:03Z

Sources/Features/Network/MultipartRequest.swift

+                try writeToFile(fileHandle, "--\(boundary)\r\n")
+                try writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attachment.filename)\"; filename=\"\(attachment.filename)\"\r\n")
+                try writeToFile(fileHandle, "Content-Type: \(attachment.mimeType)\r\n\r\n")


I think we should do it with a single write

konraddysput · 2025-01-16T12:09:30Z

Sources/Features/Network/MultipartRequest.swift

+                try writeToFile(fileHandle, "--\(boundary)\r\n")
+                try writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attachment.filename)\"; filename=\"\(attachment.filename)\"\r\n")
+                try writeToFile(fileHandle, "Content-Type: \(attachment.mimeType)\r\n\r\n")
+                fileHandle.write(attachment.data)


you're making an attachment copy in the temporary file. WE cannot stream a file directly from the file available somewhere?

possible but requires creating an InputStream for each attachments.
If we decide to keep files, i'll make the necessary improvements.

melekr · 2025-01-16T16:41:35Z

file handling, cleaning and integrity checks to be added id we decide to keep it.
Error granularity improvements to be added

perf2711 · 2025-01-17T09:42:04Z

Sources/Features/Error/BacktraceError.swift

-        case .malformedUrl(let url): return "Provided URL cannot be parsed: \(url)."
-        case .unknownError: return "Unknown error occurred."
+        case .malformedUrl(let url):
+            return "Provided URL cannot be parsed: \(url)."


small nit: can we either get rid of trailing dots or put the URL in quotes?
this goes for every description that has some data displayed in the string

perf2711 · 2025-01-17T09:58:48Z

Sources/Features/Network/MultipartRequest.swift

+            multipartRequest.setValue("\(fileSize)", forHTTPHeaderField: "Content-Length")
+
+            // Attach file stream to HTTP body
+            multipartRequest.httpBodyStream = InputStream(url: tempURL)


Instead of creating the file and setting it as httpBodyStream, couldn't you write directly to the request?

I'm not proficient in Swift, but I found this:
https://developer.apple.com/documentation/foundation/url_loading_system/uploading_streams_of_data

You can use bound streams to achieve that, it seems.

we can, this is the the approach I was referencing:

Holding data in memory for attributes or attachments changes the purpose of writing into files. A hybrid approach would be buffering data in chunks, using a buffered stream.

konraddysput · 2025-01-20T08:24:36Z

Sources/Features/Network/MultipartRequest.swift

+            try writeToStream(outputStream, reportString, writeLock: writeLock)
+            try writeLock.sync {
+                let data = report.reportData
+                let bytesWritten = data.withUnsafeBytes {


why do we need to execute this part of the code in the sync context? You won't submit two identical reports.

konraddysput · 2025-01-20T08:25:41Z

Sources/Features/Network/MultipartRequest.swift


+            // Data from Output Stream
+            guard let data = outputStream.property(forKey: .dataWrittenToMemoryStreamKey) as? Data else {


do we read data from the output stream here?

melekr added 7 commits December 17, 2024 19:04

Update Attachment

b422cea

- Migrate to modern UniformTypeIdentifiers API, keep legacy support - Cleanup

ExampleApp testing sample code

ec3f888

Update MACOSX_DEPLOYMENT_TARGET to v.11 Big Sur

63f19f0

Update mimeTypeForPath method for tvOS 14.0 & macOS 11.0

7983155

Generate live report when reporting without explicit exception

b447b73

Update ViewController.m

84619eb

melekr marked this pull request as ready for review January 6, 2025 23:44

melekr requested a review from konraddysput January 6, 2025 23:44

melekr self-assigned this Jan 6, 2025

konraddysput requested changes Jan 7, 2025

View reviewed changes

melekr requested a review from konraddysput January 7, 2025 22:37

konraddysput reviewed Jan 10, 2025

View reviewed changes

melekr added 4 commits January 14, 2025 16:41

Update MultipartRequest with file creation checks and error handling

c2fb53a

MultipartRequest error granularity

ef88290

Remove extra try when generating Live Report

e7e659e

Revert MACOSX_DEPLOYMENT_TARGET to v10.13

e5cbedb

melekr added 2 commits January 14, 2025 17:43

Reduce writeToFile operations

bb3f7f6

Cleanup

e1cd517

konraddysput reviewed Jan 16, 2025

View reviewed changes

melekr requested a review from perf2711 January 16, 2025 16:22

perf2711 reviewed Jan 17, 2025

View reviewed changes

Replace file writes with OutputStream

4c2fbde

konraddysput reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/attachments and requests #147

Improvement/attachments and requests #147

melekr commented Dec 18, 2024 •

edited

Loading

konraddysput left a comment

konraddysput Jan 7, 2025

melekr Jan 7, 2025

konraddysput Jan 9, 2025

melekr Jan 14, 2025

konraddysput Jan 7, 2025

melekr Jan 14, 2025

konraddysput Jan 7, 2025

melekr Jan 7, 2025

melekr Jan 7, 2025

konraddysput Jan 7, 2025

melekr Jan 7, 2025

konraddysput Jan 9, 2025

konraddysput Jan 9, 2025

melekr Jan 10, 2025 •

edited

Loading

melekr Jan 14, 2025

melekr commented Jan 14, 2025

konraddysput Jan 16, 2025

melekr Jan 16, 2025

konraddysput Jan 16, 2025

konraddysput Jan 16, 2025

konraddysput Jan 16, 2025

konraddysput Jan 16, 2025 •

edited

Loading

melekr Jan 16, 2025

melekr commented Jan 16, 2025

perf2711 Jan 17, 2025

perf2711 Jan 17, 2025

melekr Jan 17, 2025

konraddysput Jan 20, 2025

konraddysput Jan 20, 2025


		// Data from Output Stream
		guard let data = outputStream.property(forKey: .dataWrittenToMemoryStreamKey) as? Data else {

Improvement/attachments and requests #147

Are you sure you want to change the base?

Improvement/attachments and requests #147

Conversation

melekr commented Dec 18, 2024 • edited Loading

Why?

Changes

konraddysput left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekr Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Benefits:

Caviats:

Choose a reason for hiding this comment

melekr commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konraddysput Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekr commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekr commented Dec 18, 2024 •

edited

Loading

melekr Jan 10, 2025 •

edited

Loading

konraddysput Jan 16, 2025 •

edited

Loading