ReactiveCocoa does not work well with NewRelic agent (or vice versa) #1561

Closed
nikita-leonov opened this Issue Nov 8, 2014 · 35 comments

Comments

Projects
None yet
@nikita-leonov
Contributor

nikita-leonov commented Nov 8, 2014

I didn't investigate it deeply yet, but due to invocation swizzling in RAC and some internal behaviors in NewRelic (http://newrelic.com) agent cause application to froze right after launch. Apparently it is going into infinity loop:

I reported it to NewRelic as well with issue id 117914

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 10, 2014

Member

Without seeing the code, it's hard to know where the issue specifically lies. However, an easy workaround would be to avoid the use of -rac_signalForSelector: on affected classes.

Member

jspahrsummers commented Nov 10, 2014

Without seeing the code, it's hard to know where the issue specifically lies. However, an easy workaround would be to avoid the use of -rac_signalForSelector: on affected classes.

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 10, 2014

Contributor

I think that not using rac_signalForSelector will affect our architecture in a negative way. I would rather fix this issue. As NewRelic is closed-source I am not able to fix it myself, but I already started work with NewRelic team and provided them necessary prerequisites to reproduce and fix. Working on a simplified application to reproduce an issue as well.

Contributor

nikita-leonov commented Nov 10, 2014

I think that not using rac_signalForSelector will affect our architecture in a negative way. I would rather fix this issue. As NewRelic is closed-source I am not able to fix it myself, but I already started work with NewRelic team and provided them necessary prerequisites to reproduce and fix. Working on a simplified application to reproduce an issue as well.

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 19, 2014

Contributor

I was able to reproduce issue and delivered example application to Newrelic team. There is no answer yet why it is happening, and it is hard to investigate on what side things goes wrong, as loop happening after RAC swizzling in Newrelic code. The code to reproduce is as simple as:

    func application(application: UIApplication, didFinishLaunchingWithOptions launchOptions: [NSObject: AnyObject]?) -> Bool {        
        NewRelic.startWithApplicationToken("put your newrelice token here")
        window?.rootViewController?.rac_signalForSelector("viewDidAppear:")
        return true
    }

Newrelic team aware about this issue, so if there will be some issues related to RAC I think they will follow up here.

Contributor

nikita-leonov commented Nov 19, 2014

I was able to reproduce issue and delivered example application to Newrelic team. There is no answer yet why it is happening, and it is hard to investigate on what side things goes wrong, as loop happening after RAC swizzling in Newrelic code. The code to reproduce is as simple as:

    func application(application: UIApplication, didFinishLaunchingWithOptions launchOptions: [NSObject: AnyObject]?) -> Bool {        
        NewRelic.startWithApplicationToken("put your newrelice token here")
        window?.rootViewController?.rac_signalForSelector("viewDidAppear:")
        return true
    }

Newrelic team aware about this issue, so if there will be some issues related to RAC I think they will follow up here.

@PrideChung

This comment has been minimized.

Show comment
Hide comment
@PrideChung

PrideChung Nov 19, 2014

It looks like RAC and Newrelic swizzling a same method and somehow causing an infinite loop. I encountered a similar problem while using RAC and BlocksKit together before and it's hard to solve. People claim method swizzling is evil for a reason.

It looks like RAC and Newrelic swizzling a same method and somehow causing an infinite loop. I encountered a similar problem while using RAC and BlocksKit together before and it's hard to solve. People claim method swizzling is evil for a reason.

@powerje

This comment has been minimized.

Show comment
Hide comment
@powerje

powerje Nov 19, 2014

I'm not sure if this is helpful or not but I think Aspects had a similar issue with New Relic:
steipete/Aspects#21

It looks like @steipete's fix will be to re-implement how Aspects performs method swizzling based on this New Relic blog post: http://blog.newrelic.com/2014/04/16/right-way-to-swizzle/

powerje commented Nov 19, 2014

I'm not sure if this is helpful or not but I think Aspects had a similar issue with New Relic:
steipete/Aspects#21

It looks like @steipete's fix will be to re-implement how Aspects performs method swizzling based on this New Relic blog post: http://blog.newrelic.com/2014/04/16/right-way-to-swizzle/

@steipete

This comment has been minimized.

Show comment
Hide comment
@steipete

steipete Nov 19, 2014

Yes, they proposed a different way of swizzling which has it's own drawbacks... haven't actually gotten into updating Aspects for that, especially also because NewRelic's framework is closed source, obfuscated and also protected against reverse engineering - so debugging with it is next to impossible. Their support also ignored my issues for the longest time.

Yes, they proposed a different way of swizzling which has it's own drawbacks... haven't actually gotten into updating Aspects for that, especially also because NewRelic's framework is closed source, obfuscated and also protected against reverse engineering - so debugging with it is next to impossible. Their support also ignored my issues for the longest time.

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 20, 2014

Contributor

According to NewRelic team swizzling in RAC done incorrectly, that causing NewRelic to fail. I requested them to post here more details about NR internal swizzling and how they see RAC swizzling to be fixed. Without that information I do not believe this task will move forward.

Contributor

nikita-leonov commented Nov 20, 2014

According to NewRelic team swizzling in RAC done incorrectly, that causing NewRelic to fail. I requested them to post here more details about NR internal swizzling and how they see RAC swizzling to be fixed. Without that information I do not believe this task will move forward.

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 20, 2014

Contributor

Actually link provided in support ticket by NewRelic are the same as @powerje posted.

Contributor

nikita-leonov commented Nov 20, 2014

Actually link provided in support ticket by NewRelic are the same as @powerje posted.

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 20, 2014

Contributor

Application that reproduce an issue available here— https://github.com/BuddyHOPP/NewrelicInfiniteLoopIssue

Contributor

nikita-leonov commented Nov 20, 2014

Application that reproduce an issue available here— https://github.com/BuddyHOPP/NewrelicInfiniteLoopIssue

@buchananbryce

This comment has been minimized.

Show comment
Hide comment
@buchananbryce

buchananbryce Nov 20, 2014

The issue lies in how New Relic dynamically swizzles system methods. It depends on the method's _cmd parameter to match the system's default signature. The best way to solve this problem is to ensure the cmd value of swizzled methods isn't populated with "@selector(rac_alias<method_name>)" and instead populated with "@selector(<method_name>)". Not sure if this is feasible with the way reactive cocoa does swizzling.

The issue lies in how New Relic dynamically swizzles system methods. It depends on the method's _cmd parameter to match the system's default signature. The best way to solve this problem is to ensure the cmd value of swizzled methods isn't populated with "@selector(rac_alias<method_name>)" and instead populated with "@selector(<method_name>)". Not sure if this is feasible with the way reactive cocoa does swizzling.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 20, 2014

Member

There's a really long-winded explanation here, but the TLDR is that it's really not possible for us to affect _cmd in the necessary way. -signalForSelector: requires a lot of tricks to work, especially in the presence of KVO.

@nikita-leonov As I mentioned above, it's probably in your best interest to avoid it. But you only need to avoid it for classes affected by NewRelic.

/cc @kastiglione @Coneko @joshvera @keithduncan who know a lot about the swizzling too

Member

jspahrsummers commented Nov 20, 2014

There's a really long-winded explanation here, but the TLDR is that it's really not possible for us to affect _cmd in the necessary way. -signalForSelector: requires a lot of tricks to work, especially in the presence of KVO.

@nikita-leonov As I mentioned above, it's probably in your best interest to avoid it. But you only need to avoid it for classes affected by NewRelic.

/cc @kastiglione @Coneko @joshvera @keithduncan who know a lot about the swizzling too

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 20, 2014

Contributor

@jspahrsummers unfortunately it is almost like say "do not use signalForSelector at all" It will also cause us to refactor a lot of things that are already in place.

It will be interesting to know actually details why RAC is incompatible with approach that NewRelic suggested, as it looks pretty legit.

Contributor

nikita-leonov commented Nov 20, 2014

@jspahrsummers unfortunately it is almost like say "do not use signalForSelector at all" It will also cause us to refactor a lot of things that are already in place.

It will be interesting to know actually details why RAC is incompatible with approach that NewRelic suggested, as it looks pretty legit.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 20, 2014

Member

unfortunately it is almost like say "do not use signalForSelector at all"

@nikita-leonov Does NewRelic document what they swizzle?

Member

kastiglione commented Nov 20, 2014

unfortunately it is almost like say "do not use signalForSelector at all"

@nikita-leonov Does NewRelic document what they swizzle?

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
@nikita-leonov

nikita-leonov Nov 20, 2014

Contributor

@kastiglione it would be nice question to ask, I will do so. In my understanding they swizzle almost everything, as they measure time of execution of calls, but I actually do not know how deep they are going down to. I will post here more information, as it may be helpful for temporary workaround.

Contributor

nikita-leonov commented Nov 20, 2014

@kastiglione it would be nice question to ask, I will do so. In my understanding they swizzle almost everything, as they measure time of execution of calls, but I actually do not know how deep they are going down to. I will post here more information, as it may be helpful for temporary workaround.

@buchananbryce

This comment has been minimized.

Show comment
Hide comment
@buchananbryce

buchananbryce Nov 20, 2014

Here's a table of the things we swizzle. We will also swizzle anything that subclasses these methods. https://docs.newrelic.com/docs/mobile-monitoring/mobile-sdk-api/new-relic-mobile-sdk-api/working-ios-sdk-api#instrumenting

Here's a table of the things we swizzle. We will also swizzle anything that subclasses these methods. https://docs.newrelic.com/docs/mobile-monitoring/mobile-sdk-api/new-relic-mobile-sdk-api/working-ios-sdk-api#instrumenting

@nikita-leonov

This comment has been minimized.

Show comment
Hide comment
Contributor

nikita-leonov commented Nov 20, 2014

@buchananbryce Thanks!

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

The best way to solve this problem is to ensure the _cmd value of swizzled methods isn't populated with "@selector(rac_alias_<method_name>)" and instead populated with "@selector(< method_name>)".

@buchananbryce Why not just add a check that strips "rac_alias_" prefixes? Not elegant, but I'm sure it would make happy customers. Or if that's too specific, maybe provide an API for apps to map swizzled selectors. You say yourself "The issue lies in how New Relic dynamically swizzles system methods.". I'm making lots of assumptions, but that's because I'm not much clear of "why".

Member

kastiglione commented Nov 21, 2014

The best way to solve this problem is to ensure the _cmd value of swizzled methods isn't populated with "@selector(rac_alias_<method_name>)" and instead populated with "@selector(< method_name>)".

@buchananbryce Why not just add a check that strips "rac_alias_" prefixes? Not elegant, but I'm sure it would make happy customers. Or if that's too specific, maybe provide an API for apps to map swizzled selectors. You say yourself "The issue lies in how New Relic dynamically swizzles system methods.". I'm making lots of assumptions, but that's because I'm not much clear of "why".

@buchananbryce

This comment has been minimized.

Show comment
Hide comment
@buchananbryce

buchananbryce Nov 21, 2014

It's not really feasible to add a string compare for every possible framework that could be swizzling, and again, it's not really feasible to expect any person to know which prefixes to search for... I feel like 'there be dragons' going down this rabbit hole for a solution.

The 'why' is New Relic relies on the _cmd parameter to match it's default value, to allow us to instrument a variable number of methods without any sort of user intervention to make it happen, and ideally, when swizzling, the _cmd value should always match the name of the method that's being executed.

I see that you are swizzling the forwardInvocation method; would it be possible for you to manipulate the _cmd parameter in the invocation's parameter list before forwarding it on at that point?

It's not really feasible to add a string compare for every possible framework that could be swizzling, and again, it's not really feasible to expect any person to know which prefixes to search for... I feel like 'there be dragons' going down this rabbit hole for a solution.

The 'why' is New Relic relies on the _cmd parameter to match it's default value, to allow us to instrument a variable number of methods without any sort of user intervention to make it happen, and ideally, when swizzling, the _cmd value should always match the name of the method that's being executed.

I see that you are swizzling the forwardInvocation method; would it be possible for you to manipulate the _cmd parameter in the invocation's parameter list before forwarding it on at that point?

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 21, 2014

Member

I see that you are swizzling the forwardInvocation method; would it be possible for you to manipulate the _cmd parameter in the invocation's parameter list before forwarding it on at that point?

@buchananbryce Hmm, that's a good question that hadn't occurred to me before. It'd certainly be worth a shot!

Member

jspahrsummers commented Nov 21, 2014

I see that you are swizzling the forwardInvocation method; would it be possible for you to manipulate the _cmd parameter in the invocation's parameter list before forwarding it on at that point?

@buchananbryce Hmm, that's a good question that hadn't occurred to me before. It'd certainly be worth a shot!

@jspahrsummers jspahrsummers self-assigned this Nov 21, 2014

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 21, 2014

Member

Ah, I remember now why that won't work. The issue is that, when we have a rac_alias_ selector, we use NSInvocation to invoke it with the correct conventions. That doesn't give us a hook to change _cmd before the method sees it.

The need to use NSInvocation is, in fact, the reason that we even have rac_alias_ selectors in the first place (vs. saving IMPs). We support intercepting methods of arbitrary signatures, so it's impossible to handle all the possible cases ourselves.

TLDR: I don't think there's an easy workaround here. Where it's an issue, I'm afraid that avoiding -rac_signalForSelector: is the only option. It's really not that bad to go without it—most uses can be replaced with a simple RACSubject property.

Member

jspahrsummers commented Nov 21, 2014

Ah, I remember now why that won't work. The issue is that, when we have a rac_alias_ selector, we use NSInvocation to invoke it with the correct conventions. That doesn't give us a hook to change _cmd before the method sees it.

The need to use NSInvocation is, in fact, the reason that we even have rac_alias_ selectors in the first place (vs. saving IMPs). We support intercepting methods of arbitrary signatures, so it's impossible to handle all the possible cases ourselves.

TLDR: I don't think there's an easy workaround here. Where it's an issue, I'm afraid that avoiding -rac_signalForSelector: is the only option. It's really not that bad to go without it—most uses can be replaced with a simple RACSubject property.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

The code set the invocation's selector to the RAC alias. See NSObject+RACSelectorSignal.m#L49. EDIT: Yeah, what @jspahrsummers said.

Member

kastiglione commented Nov 21, 2014

The code set the invocation's selector to the RAC alias. See NSObject+RACSelectorSignal.m#L49. EDIT: Yeah, what @jspahrsummers said.

@jspahrsummers jspahrsummers removed their assignment Nov 21, 2014

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

It would be great if NSInvocation made -invokeUsingIMP: public, that might solve our problems.

Member

kastiglione commented Nov 21, 2014

It would be great if NSInvocation made -invokeUsingIMP: public, that might solve our problems.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

We support intercepting methods of arbitrary signatures, so it's impossible to handle all the possible cases ourselves.

The thought I had on this is we could special case a handful of very common signatures. Somewhat like the block trampoline, except maybe with the addition of handling some non-object types. If the selector is irregular, we resort to _objc_msgForward.

From New Relic's docs, the signatures on UIViewController are twofold: void -> void and BOOL -> void.

Member

kastiglione commented Nov 21, 2014

We support intercepting methods of arbitrary signatures, so it's impossible to handle all the possible cases ourselves.

The thought I had on this is we could special case a handful of very common signatures. Somewhat like the block trampoline, except maybe with the addition of handling some non-object types. If the selector is irregular, we resort to _objc_msgForward.

From New Relic's docs, the signatures on UIViewController are twofold: void -> void and BOOL -> void.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 21, 2014

Member

@kastiglione I'm not a big fan of that inconsistency. Although the current behavior is pretty shitty with regard to _cmd, it's at least consistent across all observed selectors.

Member

jspahrsummers commented Nov 21, 2014

@kastiglione I'm not a big fan of that inconsistency. Although the current behavior is pretty shitty with regard to _cmd, it's at least consistent across all observed selectors.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

It's not really feasible to add a string compare for every possible framework that could be swizzling

@buchananbryce I feel like this may be a fallacy. If there are that many frameworks that are causing problems that you can't special case them, then seems to me you have a larger problem on your hands. If there aren't that many, then why is it not feasible?

Btw, Apple and WebKit both special case by bundle ID to work around problems, but of course that's not an apples to apples comparison.

Member

kastiglione commented Nov 21, 2014

It's not really feasible to add a string compare for every possible framework that could be swizzling

@buchananbryce I feel like this may be a fallacy. If there are that many frameworks that are causing problems that you can't special case them, then seems to me you have a larger problem on your hands. If there aren't that many, then why is it not feasible?

Btw, Apple and WebKit both special case by bundle ID to work around problems, but of course that's not an apples to apples comparison.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 21, 2014

Member

@jspahrsummers I'm just thinking of helping out RAC developers. It sucks, but at least subjects can be used as a workaround.

Member

kastiglione commented Nov 21, 2014

@jspahrsummers I'm just thinking of helping out RAC developers. It sucks, but at least subjects can be used as a workaround.

@grp

This comment has been minimized.

Show comment
Hide comment
@grp

grp Nov 25, 2014

You could fix this by replacing -[NSInvocation invoke] with libffi, no? Convert the type signature and ffi_call() the IMP you want.

grp commented Nov 25, 2014

You could fix this by replacing -[NSInvocation invoke] with libffi, no? Convert the type signature and ffi_call() the IMP you want.

@kastiglione

This comment has been minimized.

Show comment
Hide comment
@kastiglione

kastiglione Nov 25, 2014

Member

I presume something like that would work. Libffi has been mentioned a couple of times, but no serious discussion (that I've seen).

Member

kastiglione commented Nov 25, 2014

I presume something like that would work. Libffi has been mentioned a couple of times, but no serious discussion (that I've seen).

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Nov 26, 2014

Member

libffi would probably work, yes, but it's also a large, scary, and fragile dependency (especially on iOS) to bring in. I generally prefer to rely upon built-in Cocoa tooling where possible.

Member

jspahrsummers commented Nov 26, 2014

libffi would probably work, yes, but it's also a large, scary, and fragile dependency (especially on iOS) to bring in. I generally prefer to rely upon built-in Cocoa tooling where possible.

@sprynmr

This comment has been minimized.

Show comment
Hide comment
@sprynmr

sprynmr Feb 27, 2015

I assume there's been no changes on this on either side? Just ran into the same issue. We'll be reporting it to new relic, but it sounds like they aren't interested in changing it.

sprynmr commented Feb 27, 2015

I assume there's been no changes on this on either side? Just ran into the same issue. We'll be reporting it to new relic, but it sounds like they aren't interested in changing it.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Feb 27, 2015

Member

The RAC problem is extremely hard (if not impossible) to fix without libffi. The suggested workaround is to use RACSubject instead of -rac_signalForSelector: for affected code.

Member

jspahrsummers commented Feb 27, 2015

The RAC problem is extremely hard (if not impossible) to fix without libffi. The suggested workaround is to use RACSubject instead of -rac_signalForSelector: for affected code.

@sprynmr

This comment has been minimized.

Show comment
Hide comment
@sprynmr

sprynmr Feb 27, 2015

Yeah that's what we're doing. Just cumbersome. Love signalForSelector for lifecycle methods. Understood on not wanting to introduce a dependency. Maybe @buchananbryce will throw us some love and add an exception for RAC.

sprynmr commented Feb 27, 2015

Yeah that's what we're doing. Just cumbersome. Love signalForSelector for lifecycle methods. Understood on not wanting to introduce a dependency. Maybe @buchananbryce will throw us some love and add an exception for RAC.

@fightingmonk

This comment has been minimized.

Show comment
Hide comment
@fightingmonk

fightingmonk Feb 28, 2015

Hi @sprynmr. We're not opposed to adding special case support for RAC. We do have to consider the performance implications and how popular the framework is. Please do file a report / request so we've got your vote counted.

Hi @sprynmr. We're not opposed to adding special case support for RAC. We do have to consider the performance implications and how popular the framework is. Please do file a report / request so we've got your vote counted.

@sprynmr

This comment has been minimized.

Show comment
Hide comment
@sprynmr

sprynmr Mar 2, 2015

@fightingmonk Noted. We sent our request along through proper channels. Thanks!

sprynmr commented Mar 2, 2015

@fightingmonk Noted. We sent our request along through proper channels. Thanks!

@mdarnall

This comment has been minimized.

Show comment
Hide comment
@mdarnall

mdarnall Aug 7, 2015

I'm seeing this issue when using rac_signalForSelector: to manage the subscription only until a view disappears. I've filed a support case with New Relic asking for them to special case RAC.

RACSignal *willDisappear = [self rac_signalForSelector:@selector(viewWillDisappear:)];
[[mySignal takeUntil:willDisappear] subscribeNext:^(id x){}]]

mdarnall commented Aug 7, 2015

I'm seeing this issue when using rac_signalForSelector: to manage the subscription only until a view disappears. I've filed a support case with New Relic asking for them to special case RAC.

RACSignal *willDisappear = [self rac_signalForSelector:@selector(viewWillDisappear:)];
[[mySignal takeUntil:willDisappear] subscribeNext:^(id x){}]]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment