search

appsquickly / typhoon

Powerful dependency injection for Objective-C ✨✨ (https://PILGRIM.PH is the pure Swift successor to Typhoon!!)✨✨
https://pilgrim.ph
Apache License 2.0
2.7k stars 269 forks source link

Non-Thread Safe Property Injection #530

Closed vitorhugomagalhaes closed 7 years ago

vitorhugomagalhaes commented 8 years ago

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 commented 8 years ago

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 notifications@github.com написал(а):

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 commented 8 years ago

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 commented 8 years ago

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 notifications@github.com написал(а):

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 commented 8 years ago

Sure.

If you need more details, feel free to ask.

alexgarbarev commented 7 years ago

@vitorhugomagalhaes what's Typhoon version you using?

vitorhugomagalhaes commented 7 years ago

Hi,

Version 3.2.8 , maybe a bit old ...

Should I try the latest on ?

Thanks,

vitorhugomagalhaes commented 7 years ago

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 commented 7 years ago

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 notifications@github.com написал(а):

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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,