Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cicrular dependencies with prototype scope #72

Closed
jasperblues opened this issue Sep 8, 2013 · 23 comments
Closed

Cicrular dependencies with prototype scope #72

jasperblues opened this issue Sep 8, 2013 · 23 comments

Comments

@jasperblues
Copy link
Member

A complex chain of circular dependencies with prototype scope, can mean that two different instances of a dependency are instantiated .. there should only be one.

Here's an example:

- (id)yourDetailsController
{
    return [TyphoonDefinition withClass:[MPYourDetailsController class] initialization:^(TyphoonInitializer* initializer)
    {
        initializer.selector = @selector(initWithServiceClient:yourDetailsView:validator:);
        [initializer injectWithDefinition:[self registrationClient]];
        [initializer injectWithDefinition:[self yourDetailsView]];
        [initializer injectWithDefinition:[self yourDetailsValidator]];
    }];
}

- (id)yourDetailsView;
{
    return [TyphoonDefinition withClass:[MPYourDetailsView class] properties:^(TyphoonDefinition* definition)
    {
        [definition injectProperty:@selector(delegate) withDefinition:[self yourDetailsController]];
    }];
}


- (id)yourDetailsValidator
{
    return [TyphoonDefinition withClass:[MPInputValidator class] initialization:^(TyphoonInitializer* initializer)
    {
        initializer.selector = @selector(initWithView:);
        [initializer injectWithDefinition:[self yourDetailsView]];
    }];
}

The UIView given to the controller is not the same UIView given to the validator:

@jasperblues
Copy link
Member Author

Relates to #49 ?

@rhgills
Copy link
Contributor

rhgills commented Sep 11, 2013

Based on my understanding of prototype scope, this seems to be working correctly.

Each time you make a reference to a definition of prototype scope, a new instance should be created based on that definition.

In fact now that I'm thinking about this, I'm not sure if we should even be handling circular dependencies for definitions with prototype scope. It seems incorrect to do the nice thing and try to handle circular dependencies between prototypes. If prototype scope does mean that each time we reference the definition a new instance is created, why should we make an attempt to resolve circular dependencies when a definition of prototype scope is involved?

I've been wrestling with a problem myself where I have two views that are similar but not identical. In order to maintain two seperate object graphs coming off of these views, I've been using singletons but prefixing them with the name of their associated view. So I'd have two singleton definitions, detailsValidator_A and detailsValidator_B - which would have similar, if not identical, definitions. I want to get rid of this duplication, but short of defining a custom scope off of the two different view controllers or just leaving the parallel methods but deduplicating their contents, I'm at a loss.

@rhgills
Copy link
Contributor

rhgills commented Sep 11, 2013

For reducing duplication in definitions, perhaps:

-[TyphoonDefinition definitionWithBaseDefinition:init:properties:]

used like:

- (id)lockA {
  return [TyphoonDefinition definitionWithBaseDefinition:[self genericLock] properties:^(TyphoonDefinition *d) {
    d.scope = TyphoonScopeSingleton;
  }];
}

- (id)lockB {
  return [TyphoonDefinition definitionWithBaseDefinition:[self genericLock] properties:^(TyphoonDefinition *d) {
    d.scope = TyphoonScopeSingleton;
  }];
}

We could even introduce a prefix that could be applied to methods indicating that they should be ignored by the BlockComponentFactory and not used to generate instances. For example,
[self __genericLock] instead of [self genericLock].

@jasperblues
Copy link
Member Author

For two components that are similar, but with some differences, Spring allows Abstract definitions, with parent= attribute on children. . . it works quite well.

@jasperblues
Copy link
Member Author

Re prototype scopes: Spring does not allow this kind of circular reference. Eg: http://stackoverflow.com/questions/10008714/requested-bean-is-currently-in-creation-is-there-an-unresolvable-circular-refer

I thin the "weak singleton" / "shared singleton" (which is the better name? I think weak is good. ) . . . anyway I think the weak singleton would get me exactly what I want. .. I wan the object graph I described above, but I want it to go away when I'm done.

@jasperblues
Copy link
Member Author

Re: invoking methods on the Assembly. ..

It would be nice to introspect for anything that is not a definition and let that pass. . . but unfortunately objective-c only allows return type introspection for properties.

We could perhaps create an "annotation" to say this is a component. . .

typhoon_component foobarComponent
{

}

this would be a Macro that does the following:

  1. Adds the method to a static array of reserved methods.
  2. Creates the definition.

. . . anything else would go through.

(I'm not recommending these ways - just presenting them as more ideas).

@jasperblues
Copy link
Member Author

Also if a circular component is used in an initializer (contructor) Spring will throw an exception "Still under construction", regardless of scope. Logged #75 for this.

@cesteban
Copy link
Contributor

I think it is very useful to have init injection of cycles. In my every day use of Typhoon this is a must. Perhaps if you start working with Typhoon from the very beginning of the development process this is not a problem, but when trying to incorporate to an existing project, sometimes you can't just avoid cycles, and using property injection all over the app is not convenient. Of course, it's only my opinion.

Which I would try to do instead is to at least detect cycles in pure init injection mode. Right now, you know you have a cycle because Typhoon crashes.

@jasperblues
Copy link
Member Author

Ok, I've changed #75 to "Should issue a warning if circular dependent component is used in initializer". .

@jasperblues
Copy link
Member Author

I'm looking forward to implementing the new "weak singleton" scope for the example above. . . Suggested name:

TyphoonSharedPrototype?
TyphoonWeakSingleton?
something else?

This is essentially the same as singleton, but it goes away (dealloc'd) when nothing else is referencing it. . Comes back again if needed.

@rhgills
Copy link
Contributor

rhgills commented Sep 12, 2013

WeakSingleton seems perfect.

@rhgills
Copy link
Contributor

rhgills commented Sep 12, 2013

Re: annotations, I feel that opt out is better than opt in. The common case, defining a method that returns a typhoon definition that constructs a component, should be as easy as possible. That is another option, not sure if I prefer it - a method prefix is certainly simpler.

@rhgills
Copy link
Contributor

rhgills commented Sep 12, 2013

I like the abstract definition idea, that sounds like it would do what I want.

Not sure I follow that Spring example though, the custom scope code is hard to understand when reading on a phone. But I do think we should raise an exception, because consider objects of prototype scope A and B which are circularly dependent. Instantisting A would require B, which would require a new instance of A, which would require a new instance of B, and so on.

@jasperblues
Copy link
Member Author

@rhgills OK, good points. I agree with your reasoning. . . (actually those suggestions were pretty much 'straw-man' ideas, so I'm not surprised they caught fire ;) )

@cesteban
Copy link
Contributor

Yes, in the example above we should definitely raise an exception. Right now, Typhoon crashes, if I'm not wrong.

But mixing init injection and property injection in cycles should be fine. Do you agree?

@jasperblues
Copy link
Member Author

@cesteban - I was agreeing with @rhgills about the "annotations" vs method prefix in the assembly. . . in case you want to have ordinary methods. . . a different discussion.

"But mixing init injection and property injection in cycles should be fine. Do you agree?"

Yes, I've accepted that we'll just raise a warning that the object in the initializer is not in its final state.

Regarding the use of the stash:

a) We have an answer to the concurrency. . . I suggested before using @synchronized .. or maybe we could compare requesting thread with current thread.

b) Do we need to call this a new scope? We've got two possible behaviors for prototype now:

  1. Any prototype definition that is involved in a dependency chain after requesting a component becomes shared. A subsequent request returns a new instance*
  2. Any prototype definition is always a new instance. . . .

is there ever a time when we really do want option 2. ?

*point 1: This relates to the point on threading (ie it won't be possible to have multiple theads access the container. .. if someone needs this it would require a factory pool).

@rhgills
Copy link
Contributor

rhgills commented Sep 18, 2013

The idea of having a super simple prototype scope that just always creates a new instance, no matter what (and raises an exception if there is a circular dependency) appeals to me. I'm not sure we should be doing the 'nice' thing in this case and attempt to resolve a circular dependency because we've been told to do two conflicting things, by this interpretation of 'prototype'. One, that we should always create a new instance when this component is referenced. Two, that there is a circular dependency.

Behavior #1 (shared scope?) seems similar to the WeakSingleton scope, or is it just me?

@jasperblues
Copy link
Member Author

You're it is very similar - in fact exactly it. . (Although different to the way I was going to implement it). . the effect is the same though. .

So as it stands now on master:

  • TyphoonScopePrototype is behaving like TyphoonScopeWeakSingleton
  • We're missing one scope.

@jasperblues
Copy link
Member Author

. . actually its not quite the same, because what we have now only applies during the scope of a request. . I think what's their is definitely useful. . I also agree that:

  • It probably should not be called prototype
  • We need the basic vanila prototype.

So far, the only reason I wanted TyphoonScopeWeakSingleton was to get the behavior of #1. . . but @cesteban 's work already gets me there.

. . I could still see WeakSingleton being useful though.

So, I suggest we have:

TyphoonScopePrototype - the old behavior of prototype . . (but currently behaving like the next)
TyphoonScopeResolve - the new behavior that @cesteban implemented. . . anyone have a better name?
TyphoonScopeSingleton
TyphoonScopeWeakSingleton

@cesteban
Copy link
Contributor

Regarding the example of the view controller that started this thread, I would say that it exactly what I think of when you use the TyphoonScopeResolve name. It has nothing to do with the fix I implemented, which creates one instance per prototype per cycle! (not per request).

If I'm understanding your problem, what you want is to maintain a cache of prototypes that are shared while you are in an specific context (in this case the creation of the view controller). In other words, letting the user to define custom scopes. And that would be such a great feature! But probably very expensive to implement.

In your particular case, I would change my design, removing the dependency of the view validator on the view, since the view controller can provide the view to the validator as a parameter when it needs to validate (please, correct me if I misunderstood the example).

@cesteban
Copy link
Contributor

The concurrency problem really concerns me...

@jasperblues
Copy link
Member Author

@cesteban What I meant by the (in retrospect bad) name 'ScopeRequest' was the scope of a call to 'componentForKey'.

I suggest a scope with the following traits:

  • Any relationships between components within the context of _building an instance_ (ie 'componentForKey') will return a shared instance. Ie in my example above the non-singleton view will be shared between both the validator and controller.
  • These components are not retained by Typhoon. So you can build an object graph, with some circular dependencies and weak references. This whole object graph will be dealloc'd after it has been used.

And I want the chosen name of this scope to strongly communicate its purpose, without ambiguity.

I thought (incorrectly?) this is what you had worked on. It sounds like your recent commits were about fixing the case of circular dependencies between two components.

On changing design: Yes, I could (did) do that. But its also possible to change design and just set up circular relationships using 'afterPropertyInjection' (the Typhoon post-construct callback) or some other place.

I think the above will be a very useful feature. Alternatively, I think I could also do what I wanted with the subtly different TyphoonScopeWeakSingleton . . maybe we only need one such scope.

@jasperblues
Copy link
Member Author

Closing - outcome of this is we added new scopes to the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants