[WIP] Fixed concurrency crash #554
Conversation
mamunto
requested review from
nachoBonafonte,
bryce-b and
vvydier
as code owners
|
|
|
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. |
mamunto
force-pushed
the
mdalmamun/concurrency-fix
branch
from
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.
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.
I didn't do any load test here. Any concern here will require to convert to async but that will trigger more API changes
Please let me know when we can get this PR merge and possibly a release |
Inside
getAggregatorHandlefunction 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.