[ObjC] Use os_unfair_lock instead of dispatch_semaphore_t.
The minOS version is high enough it can be used, and it avoids the long standing
issue for priority inversion dependent on what callers did with
threading/queues.
diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m
index 528149f..9efef70 100644
--- a/objectivec/GPBMessage.m
+++ b/objectivec/GPBMessage.m
@@ -32,6 +32,7 @@
#import <objc/message.h>
#import <objc/runtime.h>
+#import <os/lock.h>
#import <stdatomic.h>
#import "GPBArray_PackagePrivate.h"
@@ -70,8 +71,7 @@
@package
GPBUnknownFieldSet *unknownFields_;
NSMutableDictionary *extensionMap_;
- // Readonly access to autocreatedExtensionMap_ is protected via
- // readOnlySemaphore_.
+ // Readonly access to autocreatedExtensionMap_ is protected via readOnlyLock_.
NSMutableDictionary *autocreatedExtensionMap_;
// If the object was autocreated, we remember the creator so that if we get
@@ -80,19 +80,22 @@
GPBFieldDescriptor *autocreatorField_;
GPBExtensionDescriptor *autocreatorExtension_;
- // Message can only be mutated from one thread. But some *readonly* operations
- // modify internal state because they autocreate things. The
- // autocreatedExtensionMap_ is one such structure. Access during readonly
- // operations is protected via this semaphore.
- // NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
- // pointed out that they are vulnerable to live locking on iOS in cases of
- // priority inversion:
+ // Message can only be mutated from one thread. But some *readonly* operations modify internal
+ // state because they autocreate things. The autocreatedExtensionMap_ is one such structure.
+ // Access during readonly operations is protected via this semaphore.
+ //
+ // Long ago, this was a OSSpinLock, but then it came to light that there were issues for that on
+ // iOS:
// http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
// https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
- // Use of readOnlySemaphore_ must be prefaced by a call to
- // GPBPrepareReadOnlySemaphore to ensure it has been created. This allows
- // readOnlySemaphore_ to be only created when actually needed.
- _Atomic(dispatch_semaphore_t) readOnlySemaphore_;
+ // So it was a dispatch_semaphore_t, but that has issues with priority inversion if developer code
+ // uses queue for different things. But the minOS versions are now high enough, os_unfair_lock and
+ // be used, and should provide all the support needed. For more information in the
+ // concurrence/locking space see:
+ // https://gist.github.com/tclementdev/6af616354912b0347cdf6db159c37057
+ // https://developer.apple.com/library/archive/documentation/Performance/Conceptual/EnergyGuide-iOS/PrioritizeWorkWithQoS.html
+ // https://developer.apple.com/videos/play/wwdc2017/706/
+ os_unfair_lock readOnlyLock_;
}
@end
@@ -746,33 +749,6 @@
self->autocreatorExtension_ = nil;
}
-// Call this before using the readOnlySemaphore_. This ensures it is created only once.
-void GPBPrepareReadOnlySemaphore(GPBMessage *self) {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdirect-ivar-access"
-
- // Create the semaphore on demand (rather than init) as developers might not cause them
- // to be needed, and the heap usage can add up. The atomic swap is used to avoid needing
- // another lock around creating it.
- if (self->readOnlySemaphore_ == nil) {
- dispatch_semaphore_t worker = dispatch_semaphore_create(1);
- dispatch_semaphore_t expected = nil;
- if (!atomic_compare_exchange_strong(&self->readOnlySemaphore_, &expected, worker)) {
- dispatch_release(worker);
- }
-#if defined(__clang_analyzer__)
- // The static analyzer thinks worker is leaked (doesn't seem to know about
- // atomic_compare_exchange_strong); so just for the analyzer, let it think
- // worker is also released in this case.
- else {
- dispatch_release(worker);
- }
-#endif
- }
-
-#pragma clang diagnostic pop
-}
-
static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
if (!self->unknownFields_) {
self->unknownFields_ = [[GPBUnknownFieldSet alloc] init];
@@ -845,6 +821,7 @@
if ((self = [super init])) {
messageStorage_ =
(GPBMessage_StoragePtr)(((uint8_t *)self) + class_getInstanceSize([self class]));
+ readOnlyLock_ = OS_UNFAIR_LOCK_INIT;
}
return self;
@@ -915,9 +892,6 @@
- (void)dealloc {
[self internalClear:NO];
NSCAssert(!autocreator_, @"Autocreator was not cleared before dealloc.");
- if (readOnlySemaphore_) {
- dispatch_release(readOnlySemaphore_);
- }
[super dealloc];
}
@@ -1741,8 +1715,7 @@
}
// Check for an autocreated value.
- GPBPrepareReadOnlySemaphore(self);
- dispatch_semaphore_wait(readOnlySemaphore_, DISPATCH_TIME_FOREVER);
+ os_unfair_lock_lock(&readOnlyLock_);
value = [autocreatedExtensionMap_ objectForKey:extension];
if (!value) {
// Auto create the message extensions to match normal fields.
@@ -1759,7 +1732,7 @@
[value release];
}
- dispatch_semaphore_signal(readOnlySemaphore_);
+ os_unfair_lock_unlock(&readOnlyLock_);
return value;
}
diff --git a/objectivec/GPBRootObject.m b/objectivec/GPBRootObject.m
index 08adfcf..e11bb4c 100644
--- a/objectivec/GPBRootObject.m
+++ b/objectivec/GPBRootObject.m
@@ -31,6 +31,7 @@
#import "GPBRootObject_PackagePrivate.h"
#import <objc/runtime.h>
+#import <os/lock.h>
#import <CoreFoundation/CoreFoundation.h>
@@ -96,19 +97,24 @@
return jenkins_one_at_a_time_hash(key);
}
-// NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
-// pointed out that they are vulnerable to live locking on iOS in cases of
-// priority inversion:
+// Long ago, this was a OSSpinLock, but then it came to light that there were issues for that on
+// iOS:
// http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
// https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
-static dispatch_semaphore_t gExtensionSingletonDictionarySemaphore;
+// So it was a dispatch_semaphore_t, but that has issues with priority inversion if developer code
+// uses queue for different things. But the minOS versions are now high enough, os_unfair_lock and
+// be used, and should provide all the support needed. For more information in the
+// concurrence/locking space see:
+// https://gist.github.com/tclementdev/6af616354912b0347cdf6db159c37057
+// https://developer.apple.com/library/archive/documentation/Performance/Conceptual/EnergyGuide-iOS/PrioritizeWorkWithQoS.html
+// https://developer.apple.com/videos/play/wwdc2017/706/
+static os_unfair_lock gExtensionSingletonDictionaryLock = OS_UNFAIR_LOCK_INIT;
static CFMutableDictionaryRef gExtensionSingletonDictionary = NULL;
static GPBExtensionRegistry *gDefaultExtensionRegistry = NULL;
+ (void)initialize {
// Ensure the global is started up.
if (!gExtensionSingletonDictionary) {
- gExtensionSingletonDictionarySemaphore = dispatch_semaphore_create(1);
CFDictionaryKeyCallBacks keyCallBacks = {
// See description above for reason for using custom dictionary.
0,
@@ -139,9 +145,9 @@
+ (void)globallyRegisterExtension:(GPBExtensionDescriptor *)field {
const char *key = [field singletonNameC];
- dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore, DISPATCH_TIME_FOREVER);
+ os_unfair_lock_lock(&gExtensionSingletonDictionaryLock);
CFDictionarySetValue(gExtensionSingletonDictionary, key, field);
- dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
+ os_unfair_lock_unlock(&gExtensionSingletonDictionaryLock);
}
static id ExtensionForName(id self, SEL _cmd) {
@@ -173,20 +179,20 @@
key[classNameLen + 1 + selNameLen] = '\0';
// NOTE: Even though this method is called from another C function,
- // gExtensionSingletonDictionarySemaphore and gExtensionSingletonDictionary
+ // gExtensionSingletonDictionaryLock and gExtensionSingletonDictionary
// will always be initialized. This is because this call flow is just to
// lookup the Extension, meaning the code is calling an Extension class
// message on a Message or Root class. This guarantees that the class was
// initialized and Message classes ensure their Root was also initialized.
NSAssert(gExtensionSingletonDictionary, @"Startup order broken!");
- dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore, DISPATCH_TIME_FOREVER);
+ os_unfair_lock_lock(&gExtensionSingletonDictionaryLock);
id extension = (id)CFDictionaryGetValue(gExtensionSingletonDictionary, key);
// We can't remove the key from the dictionary here (as an optimization),
// two threads could have gone into +resolveClassMethod: for the same method,
// and ended up here; there's no way to ensure both return YES without letting
// both try to wire in the method.
- dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
+ os_unfair_lock_unlock(&gExtensionSingletonDictionaryLock);
return extension;
}
