search

open-telemetry / opentelemetry-swift

OpenTelemetry API for Swift
https://opentelemetry.io/docs/instrumentation/swift/
Apache License 2.0
207 stars 124 forks source link

[WIP] Fixed concurrency crash #554

Closed mamunto closed 1 month ago

mamunto commented 1 month ago

Inside getAggregatorHandle function we are just not doing read, there is a write as well. Here is code that's doing write operation:

            aggregatorHandles[processedAttributes] = newHandle
            aggregatorHandle = newHandle

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.

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

nachoBonafonte commented 1 month ago

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?

mamunto commented 1 month ago

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 commented 1 month ago

That looks perfect, thanks a lot

Please let me know when we can get this PR merge and possibly a release