Fix thread safety issue in HTTP exporters #481
Conversation
We were seeing occasional crashes that were caused by pendingLogRecords being accessed from multiple threads.
justinhporter
requested review from
nachoBonafonte,
bryce-b and
vvydier
as code owners
|
| @@ -14,7 +14,8 @@ public func defaultOltpHttpLoggingEndpoint() -> URL { | |||
| public class OtlpHttpLogExporter : OtlpHttpExporterBase, LogRecordExporter { | |||
|
|
|||
| var pendingLogRecords: [ReadableLogRecord] = [] | |||
|
|
|||
| let dispatchQueue = DispatchQueue(label: "OtlpHttpLogExporter Queue") | |||
There was a problem hiding this comment.
I would prefer if you imported Locks.swift file which is in other parts of the project (e.g in the sdk, but its methods are internal) and you used it instead of GCD queues like:
private let exporterLock = Lock()
There was a problem hiding this comment.
It appears that Lock is internal to OpenTelemetrySdk. Would you like me to make it public, or copy it to OpenTelemetryProtocolHttp?
There was a problem hiding this comment.
Copy in the exporter folder. Thanks.
| var sendingLogRecords: [ReadableLogRecord]! | ||
| dispatchQueue.sync { | ||
| pendingLogRecords.append(contentsOf: logRecords) | ||
| sendingLogRecords = pendingLogRecords | ||
| pendingLogRecords = [] | ||
| } |
There was a problem hiding this comment.
instead of this you can use:
var sendingLogRecords: [ReadableLogRecord] = []
exporterLock.withLockVoid {
pendingLogRecords.append(contentsOf: logRecords)
sendingLogRecords = pendingLogRecords
pendingLogRecords = []
}
| let body = Opentelemetry_Proto_Collector_Logs_V1_ExportLogsServiceRequest.with { request in | ||
| request.resourceLogs = LogRecordAdapter.toProtoResourceRecordLog(logRecordList: sendingLogRecords) | ||
| } | ||
|
|
||
| var request = createRequest(body: body, endpoint: endpoint) | ||
| request.timeoutInterval = min(explicitTimeout ?? TimeInterval.greatestFiniteMagnitude , config.timeout) | ||
| httpClient.send(request: request) { [weak self] result in | ||
| guard let self = self else { return } |
There was a problem hiding this comment.
this is wrong, we would ideally like to know if there was an error, which now is not printing anymore
| switch result { | ||
| case .success(_): | ||
| break | ||
| case .failure(let error): | ||
| self?.pendingLogRecords.append(contentsOf: sendingLogRecords) | ||
| self.dispatchQueue.sync { [weak self] in |
There was a problem hiding this comment.
Same with the exporterLock.withLockVoid
| @@ -52,7 +59,10 @@ public class OtlpHttpLogExporter : OtlpHttpExporterBase, LogRecordExporter { | |||
|
|
|||
| public func flush(explicitTimeout: TimeInterval? = nil) -> ExportResult { | |||
| var exporterResult: ExportResult = .success | |||
|
|
|||
| var pendingLogRecords: [ReadableLogRecord]! | |||
There was a problem hiding this comment.
same with the exporterLock.withLockVoid
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
|
You will have to sign the easyCLA before the PR can be merged. |
|
Any updates about the CLA @justinhporter ? |
We're working on getting it signed. |
|
Any updates @justinhporter ? |
Sorry, it's taking longer than I expected to get sign off from our legal department. People are out this week, but I'll try again after Thanksgiving. |
|
@justinhporter any progress on getting the CLA signed? If you can't get it done soon, we'll have to recreate the pr to get this fixed. |
Still trying, but at this point feel free to recreate this PR. |
We were seeing occasional crashes that were caused by pendingLogRecords being accessed from multiple threads. After making these changes, the crashes went away.