-
Notifications
You must be signed in to change notification settings - Fork 139
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
[WIP] Fixed concurrency crash #554
[WIP] Fixed concurrency crash #554
Conversation
|
Yes, you are true, it was an oversight and there is indeed a potential crash. If "get" cannot run concurrently is better to use serial queue with sync, as is basically what is doing it now using barrier in both. Would you mind doing that change instead? |
Yes, I can do but wanted some feedback from the team incase there is any other thoughts. Will push the change. |
c1f6d0b
to
636ce19
Compare
@@ -20,7 +20,7 @@ public class SynchronousMetricStorage: SynchronousMetricStorageProtocol { | |||
var aggregatorHandles = [[String: AttributeValue]: AggregatorHandle]() | |||
let attributeProcessor: AttributeProcessor | |||
var aggregatorHandlePool = [AggregatorHandle]() | |||
private let aggregatorHandlesQueue = DispatchQueue(label: "org.opentelemetry.SynchronousMetricStorage.aggregatorHandlesQueue", attributes: .concurrent) | |||
private let aggregatorHandlesQueue = DispatchQueue(label: "org.opentelemetry.SynchronousMetricStorage.aggregatorHandlesQueue") |
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.
Made the queue serial
@@ -76,7 +76,7 @@ public class SynchronousMetricStorage: SynchronousMetricStorageProtocol { | |||
|
|||
var points = [PointData]() | |||
|
|||
aggregatorHandlesQueue.sync(flags: .barrier) { | |||
aggregatorHandlesQueue.sync { |
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 didn't do any load test here. Any concern here will require to convert to async but that will trigger more API changes
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.
That looks perfect, thanks a lot
Please let me know when we can get this PR merge and possibly a release |
Inside
getAggregatorHandle
function we are just not doing read, there is a write as well. Here is code that's doing write operation:Concurrency queue could be overkill here, better to use a serial queue with sync or async (we will need more code changes for return) is much better option.