Fix thread race conditions in new metrics #871
Conversation
Updated SDK locks to use a more similar to Mutex approach, which allows being used in the future when we transition to Swift 6 Removed inout to properties that dont need it. Updated some properties to constants when mutability is not needed Cleaned also most warnings
nachoBonafonte
requested review from
bryce-b,
vvydier and
ArielDemarco
as code owners
Co-authored-by: ArielDemarco <[email protected]>
| @@ -40,8 +40,8 @@ public extension InstrumentBuilder { | |||
| return self | |||
| } | |||
|
|
|||
| internal func swapBuilder<T: InstrumentBuilder>(_ builder: (inout MeterProviderSharedState, inout MeterSharedState, String, String, String) -> T) -> T { | |||
There was a problem hiding this comment.
I believe if these are not inout then the internal readerStorageRegisteries and callbackRegistration will not be shared and updated in the MeterProvider
There was a problem hiding this comment.
But they are classes aren't they, the instances themselves are not changing, I thought it was a remaining if they were structs during development.
Are there any test for it?
There was a problem hiding this comment.
After doing some testing, this appears to not be the case? I remember adding inout for a specific reason that was related to the propagation of these objects.
There was a problem hiding this comment.
Did you initially made them structs, and changed later to classes? If they were structs inout would be needed.
There was a problem hiding this comment.
yes, that may have been the issue.
Updated SDK locks to use a more similar to Apple Concurrency's Mutex approach, which allows being used in the future when we transition to Swift 6. It also allows the compiler to help with the errors as the type itself includes the lock.
Removed inout to properties that dont need it.
Updated some properties to constants when mutability is not needed
Cleaned also most warnings