-
Notifications
You must be signed in to change notification settings - Fork 17
Thread aware live-report capture #158
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 1 commit
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 |
---|---|---|
|
@@ -54,6 +54,25 @@ extension BacktraceCrashReporter: CrashReporting { | |
let reportData = try reporter.generateLiveReport(with: exception) | ||
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 think it would be better if you will stick to one implementation for everything. The generateLiveReport (no thread argument) can call generateLiveReport with a thread argument by using current thread. You don't need to have a duplicated logic. If you start extending it at some point, you would need to double your code 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. sure thing, motivation was offering both methods without introducing a breaking change. |
||
return try BacktraceReport(report: reportData, attributes: attributes, attachmentPaths: attachmentPaths) | ||
} | ||
|
||
func generateLiveReport(exception: NSException? = nil, | ||
thread: mach_port_t = mach_thread_self(), | ||
attributes: Attributes, | ||
attachmentPaths: [String] = []) throws -> BacktraceReport { | ||
|
||
defer { mach_port_deallocate(mach_task_self_, thread) } | ||
let reportData = try reporter.generateLiveReport(withThread: thread, exception: exception) | ||
|
||
return try BacktraceReport(report: reportData, attributes: attributes, attachmentPaths: attachmentPaths) | ||
} | ||
|
||
// Returns the thread ID | ||
// can be used for logging/testing | ||
static func currentThreadIdentifier() -> thread_t { | ||
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. shouldn't this be a helper? We already had a lot of code in Common. This class should be as clean as possible since this is a business logic of our SDK 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. Common is for example app, removing this method since it's useless after consolidating generateLiveReport methods. |
||
let tid = mach_thread_self() | ||
mach_port_deallocate(mach_task_self_, tid) | ||
return tid | ||
} | ||
|
||
func enableCrashReporting() throws { | ||
try reporter.enableAndReturnError() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import Quick | ||
import Nimble | ||
import CrashReporter | ||
@testable import Backtrace | ||
|
||
final class BacktraceReporterThreadSpec: QuickSpec { | ||
|
||
override func spec() { | ||
|
||
describe("Thread-aware live-report capture") { | ||
|
||
var backtraceReporter: BacktraceReporter! | ||
var mockPLCR: PLCrashReporterMock! | ||
var urlSession: URLSessionMock! | ||
var credentials: BacktraceCredentials! | ||
|
||
beforeEach { | ||
credentials = BacktraceCredentials( | ||
endpoint: URL(string: "https://example.backtrace.io")!, | ||
token: "" | ||
) | ||
urlSession = URLSessionMock() | ||
mockPLCR = PLCrashReporterMock() | ||
|
||
let crashReporter = BacktraceCrashReporter(reporter: mockPLCR) | ||
|
||
let api = BacktraceApi( | ||
credentials: credentials, | ||
session: urlSession, | ||
reportsPerMin: 30 | ||
) | ||
|
||
backtraceReporter = try! BacktraceReporter( | ||
reporter: crashReporter, | ||
api: api, | ||
dbSettings: BacktraceDatabaseSettings(), | ||
credentials: credentials, | ||
urlSession: urlSession | ||
) | ||
} | ||
|
||
it("passes the calling thread to PLCrashReporter") { | ||
let callThread = mach_thread_self() | ||
_ = try? backtraceReporter.generate() | ||
expect(mockPLCR.lastThread).to(equal(callThread)) | ||
mach_port_deallocate(mach_task_self_, callThread) | ||
} | ||
|
||
it("does not leak Mach SEND rights (> 2 refs to cover noise from Quick/Nimble or XCTest)") { | ||
let before = sendRefCount() | ||
_ = try? backtraceReporter.generate() | ||
let after = sendRefCount() | ||
expect(after).to(beLessThanOrEqualTo(before + 2)) | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// Mock PLCrashReporter stubs that records the thread parameter | ||
private final class PLCrashReporterMock: PLCrashReporter { | ||
|
||
private(set) var lastThread: thread_t = mach_thread_self() | ||
|
||
override func generateLiveReport( | ||
withThread thread: thread_t, | ||
exception: NSException? | ||
) throws -> Data { | ||
lastThread = thread | ||
return Data([0xCA, 0xFE]) | ||
} | ||
|
||
override func generateLiveReport(with exception: NSException?) throws -> Data { | ||
return Data([0xBE, 0xEF]) | ||
} | ||
} | ||
|
||
/// Current ref-count for thread | ||
private func sendRefCount() -> mach_port_urefs_t { | ||
var refs: mach_port_urefs_t = 0 | ||
mach_port_get_refs( | ||
mach_task_self_, | ||
mach_thread_self(), | ||
MACH_PORT_RIGHT_SEND, | ||
&refs | ||
) | ||
return refs | ||
} |
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.
Should we always use the main thread id instead?
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.
No, we want to capture the specific currently executing thread, the main thread would be missing context and/or reporting wrong informations if the error occurs on a background thread for example.
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.