canton7 / Stylet

A very lightweight but powerful ViewModel-First MVVM framework for WPF for .NET Framework and .NET Core, inspired by Caliburn.Micro.
MIT License
992 stars 143 forks source link

Func Factories cause a StackOverflow exception #24

Open RyanYANG52 opened 7 years ago

RyanYANG52 commented 7 years ago

First of all, awesome project, thumb up, hope you continue this project.

I'm in a unique situation that I need a child view model to create a new parent view model; the parent view model is just a kind of content container, so it can create child view model dynamically according to the input model info. Constructors look like this:

ParentViewModel(Func<ChildViewModel> childViewModelFactory)
ChildViewModel(Func<ParentViewModel> parentViewModelFactory)

When build it'll cause a StackOverflow. I've read "Circular Dependencies" wiki, but i still do not know how to configure a Func Factory.

Then I try to use Abstract Factories, it can avoid the stackoverflow exception, but I still wonder why Func Factory fails in this scenario.

Thank you in advance

canton7 commented 7 years ago

I think it probably gets caught in this cycle:

  1. It tries to find out how to construct a ParentViewModel
  2. To do this, it needs to work out how to construct a ChildViewModel
  3. In order to construct a ChildViewModel, it needs to know how to construct a ParentViewModel
  4. etc

I'll investigate when I get some time (slightly snowed under at the moment, sorry), and see if there's an easy fix.

RyanYANG52 commented 7 years ago

No problem, I can use Abstract Factories at the moment

canton7 commented 7 years ago

You could also do something like builder.Bind<Func<ChildViewModel>>().ToFactory(c => () => c.Get<ChildViewModel>()).InSingletonScope() (and the same for ParentViewModel), I think, then use the Func<ChildViewModel> and Func<ParentViewModel> in your constructors.

RyanYANG52 commented 7 years ago

That's exactly what i want, and it worked. Although i think i only need to configure for the parent view model, either one of them is InSingletonScope should break the deadlock. Also because my child view model is the base of every business view model in our project, lots of view models... So the code goes like this:``

builder.Bind<Func<ParentViewModel>>().ToFactory<Func<ParentViewModel>>(c => () => c.Get<ParentViewModel>()).InSingletonScope();

It appears to be working, but i don't know whether my understanding is correct

canton7 commented 7 years ago

The InSingletonScope() gives the scope of the Func, rather than the scope of the ViewModel. It's saying "the first time you need to construct a Func<ParentViewModel>, this is how you do it, then cache the Func<ParentViewModel> instance you just created and use it again next time". It avoids instantiating a new Func each time - that's all.

The ParentViewModel instance is always transient in this case. If you wanted, you could change this by changing how ParentViewModel is bound, i.e. builder.Bind<ParentViewModel>().ToSelf().InSingletonScope(). Then container.Get<ParentViewModel>() would always return the same ParentViewModel instance, and so the Func<ParentViewModel> would always return the same instance.

I hope that makes sense, rather than confusing things further...

canton7 commented 7 years ago

Yeah, I was right in my guess as to what's going on.

When it tries to create that Func itself, it's trying to do something like () => new ParentViewModel(() => new ChildViewModel(() => new ParentViewModel(() => new ChildViewModel...., and blowing the stack in the process.

It could instead do something like () => new ParentViewModel(() => container.Get<ChildViewModel()). The downside is that it's slightly more expensive at runtime, and more importantly if it doesn't know how to construct a ChildViewModel you'll get a failure when you try to invoke the Func, rather than when you try to get a ParentViewModel instance.

So, trade-offs. Giving better error messages vs stackoverflowing in some rare cases. I'm currently tempted to keep things as they are and document this case, but I'm still thinking about it. Open to feedback!

RyanYANG52 commented 7 years ago

that's a lot to take in, -.-

Our project in short is like a TabControl inside a TabControl, all the TabItem is ParentViewModel and inside ParentViewModel there is only one ChildViewModel, and this ChildViewModel could be a TabControl also, and so on...

I understand Func Factory and InSingletonScope() part, and I need my ParentViewModel to be transient also, because one ChildViewModel could have many ParentViewModel, they are different instances, and user can close them anytime. But I only need one Func<ParentViewModel>, just setup different model infos.

Hope you understand what i'm saying. I think the reason why I make Func<ParentViewModel> Singleton would work is that I only have one ParentViewModel class, it contains information to create different type of ChildViewModel class, it's like a browser tab, only need an address to know which site it should access.

On the other hand, why does not Abstract Factories cause a StackOverflow?

canton7 commented 7 years ago

I think I follow.

Like I said, making that Func binding singleton or transient really makes no difference. The difference is effectively this:

// Singleton:
var parentFactory = () => new ParentViewModel(container.Get<ChildViewModel>());
var c1 = new ChildViewModel(parentFactory);
var c2 = new ChildViewModel(parentFactory);

// Transient:
var c1 = new ChildViewModel(() => new ParentViewModel(container.Get<ChildViewModel>()));
var c2 = new ChildViewModel(() => new ParentViewModel(container.Get<ChildViewModel>()));

Abstract factories work because they're lazier: they call container.Get<ChildViewModel>(), rather than trying to construct a new ChildViewModel themselves (as the func factories do).

RyanYANG52 commented 7 years ago

OK, Thank you very much for the explanation, I think I got it, have a great day

canton7 commented 7 years ago

Reopening, as I want to do something about this: either fix it, or document it.

RyanYANG52 commented 7 years ago

OK