Skip to content

Commit

Permalink
Handing threading race resolving methods.
Browse files Browse the repository at this point in the history
- Don't prune the extension registry as that can lead to failures when two
  threads are racing.
- If adding the method fails, check and see if it already is bound to decide
  the return result. Deals with threading races binding the methods.
  • Loading branch information
thomasvl committed Mar 2, 2017
1 parent a7e3b0a commit 2d1c5e2
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 9 deletions.
13 changes: 11 additions & 2 deletions objectivec/GPBMessage.m
Original file line number Diff line number Diff line change
Expand Up @@ -3152,8 +3152,17 @@ + (BOOL)resolveInstanceMethod:(SEL)sel {
if (result.impToAdd) {
const char *encoding =
GPBMessageEncodingForSelector(result.encodingSelector, YES);
BOOL methodAdded = class_addMethod(descriptor.messageClass, sel,
result.impToAdd, encoding);
Class msgClass = descriptor.messageClass;
BOOL methodAdded = class_addMethod(msgClass, sel, result.impToAdd, encoding);
// class_addMethod() is documented as also failing if the method was already
// added; so we check if the method is already there and return success so
// the method dispatch will still happen. Why would it already be added?
// Two threads could cause the same method to be bound at the same time,
// but only one will actually bind it; the other still needs to return true
// so things will dispatch.
if (!methodAdded) {
methodAdded = GPBClassHasSel(msgClass, sel);
}
return methodAdded;
}
return [super resolveInstanceMethod:sel];
Expand Down
21 changes: 14 additions & 7 deletions objectivec/GPBRootObject.m
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,10 @@ static id ExtensionForName(id self, SEL _cmd) {
dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore,
DISPATCH_TIME_FOREVER);
id extension = (id)CFDictionaryGetValue(gExtensionSingletonDictionary, key);
if (extension) {
// The method is getting wired in to the class, so no need to keep it in
// the dictionary.
CFDictionaryRemoveValue(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);
return extension;
}
Expand All @@ -212,9 +211,17 @@ BOOL GPBResolveExtensionClassMethod(Class self, SEL sel) {
#pragma unused(obj)
return extension;
});
if (class_addMethod(metaClass, sel, imp, encoding)) {
return YES;
BOOL methodAdded = class_addMethod(metaClass, sel, imp, encoding);
// class_addMethod() is documented as also failing if the method was already
// added; so we check if the method is already there and return success so
// the method dispatch will still happen. Why would it already be added?
// Two threads could cause the same method to be bound at the same time,
// but only one will actually bind it; the other still needs to return true
// so things will dispatch.
if (!methodAdded) {
methodAdded = GPBClassHasSel(metaClass, sel);
}
return methodAdded;
}
return NO;
}
Expand Down
19 changes: 19 additions & 0 deletions objectivec/GPBUtilities.m
Original file line number Diff line number Diff line change
Expand Up @@ -1893,6 +1893,25 @@ static int32_t ReadRawVarint32FromData(const uint8_t **data) {

#pragma clang diagnostic pop

BOOL GPBClassHasSel(Class aClass, SEL sel) {
// NOTE: We have to use class_copyMethodList, all other runtime method
// lookups actually also resolve the method implementation and this
// is called from within those methods.

BOOL result = NO;
unsigned int methodCount = 0;
Method *methodList = class_copyMethodList(aClass, &methodCount);
for (unsigned int i = 0; i < methodCount; ++i) {
SEL methodSelector = method_getName(methodList[i]);
if (methodSelector == sel) {
result = YES;
break;
}
}
free(methodList);
return result;
}

#pragma mark - GPBMessageSignatureProtocol

// A series of selectors that are used solely to get @encoding values
Expand Down
2 changes: 2 additions & 0 deletions objectivec/GPBUtilities_PackagePrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,4 +345,6 @@ GPB_MESSAGE_SIGNATURE_ENTRY(int32_t, Enum)
+ (id)getClassValue;
@end

BOOL GPBClassHasSel(Class aClass, SEL sel);

CF_EXTERN_C_END

0 comments on commit 2d1c5e2

Please sign in to comment.