Circular dependencies are broken

[drodriguez]

I don’t know if the circular dependencies code is WIP or not, but this is one problem we have found trying to use circular dependencies.

The problem can be seen in both tests about circular dependencies. I’m going to choose the first one (by_reference) but the other one shows the same behaviour.

If you change the test to read like the following code the asserts marked as “Fails” will fail:

- (void)test_resolves_circular_dependencies_for_property_injected_by_reference
{
    ClassADependsOnB* classA = [_circularDependenciesFactory componentForKey:@"classA"];
    NSLog(@"Dependency on B: %@", classA.dependencyOnB);
    assertThat(classA.dependencyOnB, notNilValue());

    // Fails
    assertThat(classA, equalTo(classA.dependencyOnB.dependencyOnA));

    id dependencyABA = classA.dependencyOnB.dependencyOnA;
    // Fails
    assertThat([[dependencyABA dependencyOnB] class], equalTo([ClassBDependsOnA class]));



    ClassBDependsOnA* classB = [_circularDependenciesFactory componentForKey:@"classB"];
    NSLog(@"Dependency on A: %@", classB.dependencyOnA);
    assertThat(classB.dependencyOnA, notNilValue());

    // Fails
    assertThat(classB, equalTo(classB.dependencyOnA.dependencyOnB));

    id dependencyBAB = classB.dependencyOnA.dependencyOnB;
    // Fails
    assertThat([[dependencyBAB dependencyOnA] class], equalTo([ClassADependsOnB class]));
}

The first assert is something that we supposed it will happen when we specified the circular dependency (if A depends on B and B depends on A, the link will be made between two instances, not more). We don’t know if it is what Typhoon is trying to solve.

The second assert is more dangerous, since a chain of modules might be providing the dependencies, and someone might end with an instance of an unexpected type. I know that it has to go through 3 jumps, but I think that creating a bad typed object graph should be avoided.

This is what I see in my debugger when I stop the test just before the first assert is run:

As you can see, the first and the second objects are created with the right types. The third object is also the right type, but instead of being the same object as the first, it is another completely different object. Finally the dependency of that object on ClassB is pointing to itself (!?) which is, obviously, of the wrong class.

From my analysis of what happens this is the sequence of steps that leads to this:

1. TyphoonSharedComponentFactoryTests.m#157 starts the process asking for a “classA”.
2. Through several calls we end in -[TyphoonComponentFactory(InstanceBuilder) doPropertyInjection:property:typeDescriptor:].
3. The first call to -[TyphoonComponentFactory(InstanceBuilder) evaluateCircularDependency:property:instance:] is made. Since _currentlyResolvingReferences is empty, the method simply updates the dictionary with @"classB": <ClassADependsOnB: 0x1060052f0> (this is strange, but seems that this dictionary tells that ClassADependsOnB is resolving "classB" reference?).
4. Going back to doPropertyInjection:property:typeDescriptor:, the code checks that this is not (yet) a circular dependent property, and proceeds with the injection.
5. Since this property is a reference to "classB", through several calls we end, again, at doPropertyInjection:property:typeDescriptor, but this time we are creating a ClassBDependsOnA and we are processing the dependencyOnA property.
6. The second call to evaluateCircularDependency:property:instance: is made. At this point _currentlyResolvingReferences only has one element (the one created in step 3), so things work more or less like before, and a new pair @"classA": <ClassBDependsOnA: 0x102446650> is added to the dictionary. I think here lies one of our problems, since we are already resolving "classA" from the start, but we never added it to the _currentlyResolvingReferences dictionary. This makes the component factory start creating another ClassADependsOnB, which is not what we expect.
7. Going back to doPropertyInjection:property:typeDescriptor:, things proceed as in step 4, and we start creating another "classA" reference.
8. Again, through several calls we end once more in doPropertyInjection:property:typeDescriptor:, trying to create a ClassADependsOnB. This time the things are different, because the circular dependency code starts being exercised.
9. The third call to evaluateCircularDependency:property:instance: is made. This time we have a "classA" in the dictionary, so this instance circularDependentProperties dictionary is updated with the pair @"dependencyOnB": @"classB". Then we overwrite the _currentlyResolvingReferences dictionary: the pair we created in step 3 now points to this second instance of ClassADependsOnB (for reference <ClassADependsOnB: 0x102446a40>).
10. This is the first time that after going back to doPropertyInjection:property:typeDescriptor: the injection doesn’t get made, and the first time we are able to “escape” doPropertyInjection:property:typeDescriptor:.
11. We end up in -[TyphoonComponentFactory(InstanceBuilder) injectCircularDependenciesOn:] for the first time. This instance has a circular dependency detected, "dependencyOnB", which references "classB". However, under that name in _currentlyResolvingReferences we have an instance of ClassADependsOnB (the one stored in step 9, and before that another one of the same class in step 3). So we end injecting a ClassADependsOnB into a dependencyOnB property. The worse thing, we stored <ClassADependsOnB: 0x102446a40> in step 9, and we are recovering it, and injecting it in itself.
12. Going back in the stack, in -[TyphoonComponentFactory componentForKey:] we proceed to remove all objects of _currentlyResolvingReferences. It doesn’t matter that much at this point, since the first two instances didn’t detect any circular references.

There are other smaller bugs related to this code that might generate confusion:

• When the injection is “delayed” because it is a circular dependency in doPropertyInjection:property:typeDescriptor:, the invocation is made anyway, which I think produces a call to the setter with a nil argument. Some classes might not like that. The invocation should be avoided if possible, or this side-effect should be documented.
• -[TyphoonComponentFactory(InstanceBuilder) doAfterPropertyInjectionOn:withDefinition:] is invoked before circular dependencies are setup, so the classes that expects those properties to be set will obtain unexpected results. The invocation of that method should be done after the circular dependencies had been injected, or the behaviour documented.

[jasperblues]

@drodriguez Thanks for the very detailed write-up. . .

Would you be interested in sending a pull-request that resolves this issue? I will gladly accept it, and contributors will be (soon) featured on the Typhoon website.

If not, I will rectify the issue in the coming days.

[drodriguez]

I was trying to come up with a fix yesterday, and, in fact the fix is pretty easy and make those modified tests pass. Then I created another test case because I wasn't sure I covered all the cases (class C depends on children D and E, which refer back to the parent). That assembly makes the populate cache recurse infinitely and I still haven't find out why (I'm trying to get my head around the auto generated method “before advice”, which I think tries to do a toposort, which will not be possible for a non-DAG, if circular dependencies are allowed).
Then I also have to test with singleton scoped definitions, but I think those work with my changes. I will try to prepare an small pull request with this change, and keep looking what happens with the complex case.

[jasperblues]

The 'before advice' is used by the block-style assembly. It is named after the Aspect Oriented Programming terminology. The purpose is just so we can use the Assembly interface, instead of fragile strings for a) Injecting components into each other b) Resolving components at runtime. That way refactoring tools etc work better.

Here's what it does:

Intercepts all method calls in the Assembly
Checks a cache to see if the required value is already there.
If not, executes the original method, and puts that in the cache. Assigns a key.
Returns the value.

I think the easiest thing to do when working on circular dependencies is ignore all of that, and use the lower-level API for the test cases. . .

Once they work, copy the tests to the TyphoonSharedComponentFactoryTests, which would be run against both the block and XML styles.

Are you using block-style assembly or XML ?

On Jun 30, 2013, at 3:54 PM, Daniel Rodríguez Troitiño [email protected] wrote:

I was trying to come up with a fix yesterday, and, in fact the fix is pretty easy and make those modified tests pass. Then I created another test case because I wasn't sure I covered all the cases (class C depends on children D and E, which refer back to the parent). That assembly makes the populate cache recurse infinitely and I still haven't find out why (I'm trying to get my head around the auto generated method “before advice”, which I think tries to do a toposort, which will not be possible for a non-DAG, if circular dependencies are allowed).
Then I also have to test with singleton scoped definitions, but I think those work with my changes. I will try to prepare an small pull request with this change, and keep looking what happens with the complex case.

—
Reply to this email directly or view it on GitHub.

[drodriguez]

We pretend to use block-style assembly. Our religious beliefs forbid us to use XML :/

About the “before advice”, that was what I understood it did. The name (“advice”) was distracting me, because I didn’t know what “advice” it was going to give me. I didn’t know that “advice” was used for this kind of functions in AOP.

So, with that in mind, I don’t get why you need to swizzle and unswizzle at this point:

[[self class] typhoon_swizzleMethod:sel withMethod:NSSelectorFromString(key) error:nil];
cached = objc_msgSend(me, sel);
if (cached && [cached isKindOfClass:[TyphoonDefinition class]])
{
  // Skipped
}
[[self class] typhoon_swizzleMethod:NSSelectorFromString(key) withMethod:sel error:nil];

Why not call directly obc_msgSend(me, sel) or obc_msgSend(me, NSSelectorFromString(key)) (I think the former)? I know how swizzle works, that you have to call “recursively” to yourself, because at that point “yourself” is actually the original method… All those swizzles are hard to follow. When I tried to follow what this method executed, it looked to me that at least the first definition, it was doing twice the call to __typhoonBeforeAdvice before calling into the real implementation. I don’t know, I will have to recheck.

[jasperblues]

This should definitely be made as simple as possible. . I think you're right. No need to swap back again, just invoke the other method.

Jasper Blues
Landline: +63.2.826.2489
Skype: jasperblues
LinkedIn Twitter

On Jun 30, 2013, at 7:59 PM, Daniel Rodríguez Troitiño [email protected] wrote:

We pretend to use block-style assembly. Our religious beliefs forbid us to use XML :/

About the “before advice”, that was what I understood it did. The name (“advice”) was distracting me, because I didn’t know what “advice” it was going to give me. I didn’t know that “advice” was used for this kind of functions in AOP.

So, with that in mind, I don’t get why you need to swizzle and unswizzle at this point:

[[self class] typhoon_swizzleMethod:sel withMethod:NSSelectorFromString(key) error:nil];
cached = objc_msgSend(me, sel);
if (cached && [cached isKindOfClass:[TyphoonDefinition class]])
{
// Skipped
}
[[self class] typhoon_swizzleMethod:NSSelectorFromString(key) withMethod:sel error:nil];
Why not call directly obc_msgSend(me, sel) or obc_msgSend(me, NSSelectorFromString(key)) (I think the former)? I know how swizzle works, that you have to call “recursively” to yourself, because at that point “yourself” is actually the original method… All those swizzles are hard to follow. When I tried to follow what this method executed, it looked to me that at least the first definition, it was doing twice the call to __typhoonBeforeAdvice before calling into the real implementation. I don’t know, I will have to recheck.

—
Reply to this email directly or view it on GitHub.

[jasperblues]

. . . still broken. . . I haven't had time to look at this yet!

[rhgills]

Fixed in #38.