Non-Thread Safe Property Injection

[vitorhugomagalhaes]

Hi,

I've discovered a race condition when the property injection code is running in parallel.

My first question, and I'm afraid of the answer :), is Typhoon thread safe ?

The code causing the issue is in TyphoonIntrospectionUtils.m line 33.

static char buffer[256];

There is no reason to be static and this is the root cause for random crashes we are facing on our app.

If Typhoon is thread safe I'd be happy to remove the static keyword. Otherwise, I'm in trouble ...

Thanks in advance,

[alexgarbarev]

Yes, typhoon supposed to be threaded safe. It's guaranteed by wrapping all outside methods code into synchronized statement, so static inside should works.
Please review your typhoon usage, which two public methods works in parallel for you?

Отправлено с iPhone

8 сент. 2016 г., в 20:41, Vitor Magalhães [email protected] написал(а):

Hi,

I've discovered a race condition when the property injection code is running in parallel.

My first question, and I'm afraid of the answer :), is Typhoon thread safe ?

The code causing the issue is in TyphoonIntrospectionUtils.m line 33.

static char buffer[256];

There is no reason to be static and this is the root cause for random crashes we are facing on our app.

If Typhoon is thread safe I'd be happy to remove the static keyword. Otherwise, I'm in trouble ...

Thanks in advance,

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[vitorhugomagalhaes]

Well, I believe that the following parallel code path is not safe.

Thread 1 (Main)

#0 0x08663dd0 in objc_exception_throw () #1 0x045d7dee in -[__NSArrayI objectAtIndex:] () #2 0x046433b8 in -[NSArray objectAtIndexedSubscript:] () #3 0x0665b45e in -[TyphoonTypeDescriptor initWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:99 #4 0x0665afaa in +[TyphoonTypeDescriptor descriptorWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:69 #5 0x0664bb7c in +[TyphoonIntrospectionUtils typeForPropertyNamed:inClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/TyphoonIntrospectionUtils.m:44 #6 0x06625170 in -[NSObject(TyphoonIntrospectionUtils) typhoonTypeForPropertyNamed:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/NSObject+TyphoonIntrospectionUtils.m:34 #7 0x06633a61 in -[TyphoonComponentFactory(InstanceBuilder) doPropertyInjectionOn:property:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonComponentFactory+InstanceBuilder.m:174 #8 0x06632fe0 in -[TyphoonComponentFactory(InstanceBuilder) doInjectionEventsOn:withDefinition:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonComponentFactory+InstanceBuilder.m:112 #9 0x06637b85 in -[TyphoonComponentFactory inject:withDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:395 #10 0x066366dc in -[TyphoonComponentFactory inject:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:249 #11 0x06658946 in -[TyphoonStoryboard injectPropertiesForViewController:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/ios/Storyboard/TyphoonStoryboard.m:138 #12 0x0665873f in -[TyphoonStoryboard instantiateViewControllerWithIdentifier:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/ios/Storyboard/TyphoonStoryboard.m:127

Thread 2 (Background)

#12 0x0665b2c7 in -[TyphoonTypeDescriptor initWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:91 #13 0x0665afaa in +[TyphoonTypeDescriptor descriptorWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:69 #14 0x0664bb7c in +[TyphoonIntrospectionUtils typeForPropertyNamed:inClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/TyphoonIntrospectionUtils.m:44 #15 0x06642077 in -[TyphoonFactoryAutoInjectionPostProcessor autoInjectedPropertiesForClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:48 #16 0x06641c72 in -[TyphoonFactoryAutoInjectionPostProcessor postProcessDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:36 #17 0x06641ba1 in -[TyphoonFactoryAutoInjectionPostProcessor postProcessDefinition:replacement:withFactory:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:29 #18 0x066372d9 in -[TyphoonComponentFactory definitionAfterApplyingPostProcessorsToDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:338 #19 0x0663703b in __46-[TyphoonComponentFactory applyPostProcessors]_block_invoke at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:325 #20 0x066363e9 in -[TyphoonComponentFactory enumerateDefinitions:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:220 #21 0x06636f83 in -[TyphoonComponentFactory applyPostProcessors] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:324 #22 0x06636ad8 in -[TyphoonComponentFactory _load] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:290 #23 0x06635580 in -[TyphoonComponentFactory load] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:108 #24 0x066361c0 in -[TyphoonComponentFactory loadIfNeeded] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:190 #25 0x0663606b in -[TyphoonComponentFactory componentForKey:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:177 #26 0x045d78bd in __invoking___ () #27 0x045d7766 in -[NSInvocation invoke] () #28 0x0467126a in -[NSInvocation invokeWithTarget:] () #29 0x066304a5 in -[TyphoonBlockComponentFactory forwardInvocation:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonBlockComponentFactory.m:111 #30 0x06627724 in -[TyphoonAssembly forwardInvocation:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Assembly/TyphoonAssembly.m:102

It seems that they are not under a common @synchronized.

Let me know if you need further information.

BR,

[alexgarbarev]

Thank you for these details! I'll take a look later today then get back to you soon.

Best Regards,
Aleksey

9 сент. 2016 г., в 12:26, Vitor Magalhães [email protected] написал(а):

Well, I believe that the following parallel code path is not safe.

*Thread 1 (Main) *

#0 0x08663dd0 in objc_exception_throw ()
#1 0x045d7dee in -__NSArrayI objectAtIndex:
#2 0x046433b8 in -NSArray objectAtIndexedSubscript:
#3 0x0665b45e in -[TyphoonTypeDescriptor initWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:99
#4 0x0665afaa in +[TyphoonTypeDescriptor descriptorWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:69
#5 0x0664bb7c in +[TyphoonIntrospectionUtils typeForPropertyNamed:inClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/TyphoonIntrospectionUtils.m:44
#6 0x06625170 in -[NSObject(TyphoonIntrospectionUtils) typhoonTypeForPropertyNamed:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/NSObject+TyphoonIntrospectionUtils.m:34
#7 0x06633a61 in -[TyphoonComponentFactory(InstanceBuilder) doPropertyInjectionOn:property:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonComponentFactory+InstanceBuilder.m:174
#8 0x06632fe0 in -[TyphoonComponentFactory(InstanceBuilder) doInjectionEventsOn:withDefinition:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonComponentFactory+InstanceBuilder.m:112
#9 0x06637b85 in -[TyphoonComponentFactory inject:withDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:395
#10 0x066366dc in -[TyphoonComponentFactory inject:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:249
#11 0x06658946 in -[TyphoonStoryboard injectPropertiesForViewController:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/ios/Storyboard/TyphoonStoryboard.m:138
#12 0x0665873f in -[TyphoonStoryboard instantiateViewControllerWithIdentifier:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/ios/Storyboard/TyphoonStoryboard.m:127

Thread 2 (Background)

#12 0x0665b2c7 in -[TyphoonTypeDescriptor initWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:91
#13 0x0665afaa in +[TyphoonTypeDescriptor descriptorWithTypeCode:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/TypeConversion/TyphoonTypeDescriptor.m:69
#14 0x0664bb7c in +[TyphoonIntrospectionUtils typeForPropertyNamed:inClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Utils/TyphoonIntrospectionUtils.m:44
#15 0x06642077 in -[TyphoonFactoryAutoInjectionPostProcessor autoInjectedPropertiesForClass:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:48
#16 0x06641c72 in -[TyphoonFactoryAutoInjectionPostProcessor postProcessDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:36
#17 0x06641ba1 in -[TyphoonFactoryAutoInjectionPostProcessor postProcessDefinition:replacement:withFactory:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Definition/AutoInjection/TyphoonFactoryAutoInjectionPostProcessor.m:29
#18 0x066372d9 in -[TyphoonComponentFactory definitionAfterApplyingPostProcessorsToDefinition:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:338
#19 0x0663703b in 46-[TyphoonComponentFactory applyPostProcessors]_block_invoke at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:325
#20 0x066363e9 in -[TyphoonComponentFactory enumerateDefinitions:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:220
#21 0x06636f83 in -[TyphoonComponentFactory applyPostProcessors] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:324
#22 0x06636ad8 in -[TyphoonComponentFactory _load] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:290
#23 0x06635580 in -[TyphoonComponentFactory load] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:108
#24 0x066361c0 in -[TyphoonComponentFactory loadIfNeeded] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:190
#25 0x0663606b in -[TyphoonComponentFactory componentForKey:args:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/TyphoonComponentFactory.m:177
#26 0x045d78bd in _invoking ()
#27 0x045d7766 in -NSInvocation invoke
#28 0x0467126a in -NSInvocation invokeWithTarget:
#29 0x066304a5 in -[TyphoonBlockComponentFactory forwardInvocation:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Internal/TyphoonBlockComponentFactory.m:111
#30 0x06627724 in -[TyphoonAssembly forwardInvocation:] at /Users/vmagalhaes/work/ndrive/nlife-ios/nlife/Pods/Typhoon/Source/Factory/Assembly/TyphoonAssembly.m:102

It seems that they are not under a common @synchronized.

Let me know if you need further information.

BR,

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[vitorhugomagalhaes]

Sure.

If you need more details, feel free to ask.

[alexgarbarev]

@vitorhugomagalhaes what's Typhoon version you using?

[vitorhugomagalhaes]

Hi,

Version 3.2.8 , maybe a bit old ...

Should I try the latest on ?

Thanks,

[vitorhugomagalhaes]

Hi,

I've being using the latest version without any crash. Cannot confirm if it really solves the issue or not.

Any feedback ?

BR,

[alexgarbarev]

Yes, I think there is still one issue in multi threading while using assembly interface. I will fix that asap.

Unfortunately these kind of problems can't be covered by unit tests or easily replicated, so we can only analyze the code. (Btw. If you have demo which shows that crash - it would be helpful)

Best Regards,
Aleksey

13 сент. 2016 г., в 18:47, Vitor Magalhães [email protected] написал(а):

Hi,

I've being using the latest version without any crash. Cannot confirm if it really solves the issue or not.

Any feedback ?

BR,

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[vitorhugomagalhaes]

Hi,

Demo is not easy. The project is quite big and it's difficult to isolate the specific code to replicate the behaviour.

Thanks,

[vitorhugomagalhaes]

Hi,

Just wondering if you have the time to check this.

Removing the static buffer seems to workaround. We will release to market next week so I'm bit short in time.

If you don't approve the static keyword removal I'll probably fork this for our selfs right now.

Any suggestion ?

Thanks in advance,

[alexgarbarev]

Hi @vitorhugomagalhaes. Yes, I'm still keeping this problem in mind, but I'm super busy these days. I found reason of problems above and know the solution. If you can make fork and PR with fix - that would be helpful a lot!

So, the problem:
I thought all external (public) methods had synchronised statements, such a

- (id)componentForType:(id)classOrProtocol;

- (id)componentForKey:(NSString *)key args:(TyphoonRuntimeArguments *)args;

methods and so on.. But when we using assembly interface to build/access instances, TyphoonComponentFactory intercept method invocation and then calls all safe methods from above. So, problem is - method invocation interception and handling is not threaded safe :-(

The solution is:
Wrap content of these method into synchronized statement:

TyphoonAssembly.m: forwardInvocation

TyphoonBlockComponentFactory.m: forwardInvocation
TyphoonBlockComponentFactory.m: methodSignatureForSelector

Plus I agree with removing static buffer statement. This micro optimisation doesn't worth problems it lead.

And good luck with the release, hope all goes well!

[vitorhugomagalhaes]

I've changed the code as requested and submitted the PR.

I'll be testing our fork during the next week.

Hopefully it won't crash anymore :)

BR,