Is there a bug related to using many items in a conductor? I need to manage a few thousand view model instances and wanted to use a Conductor<ItemViewModel>.Collection.OneActive for that, but as soon as I close the app, I get a StackOverflowException in Caliburn.Micro.dll.

Even tested it in a clean new project with very simple view models and views, but with the same result. This project crashes at about 8000 items, my actual application even sooner (about 5000-6000).

That'd be the main view model and the view model for single items:

public class MainViewModel : Conductor<ItemViewModel>.Collection.OneActive
{
    public MainViewModel()
    {
        for (int i = 0; i < 8000; ++i)
        {
            this.Items.Add(new ItemViewModel(i.ToString()));
        }
    }
}

public class ItemViewModel : PropertyChangedBase
{
    private string _name;

    public ItemViewModel(string name)
    {
        this._name = name;
    }

    public string Name
    {
        get { return this._name; }
        set
        {
            this._name = value;
            this.NotifyOfPropertyChange(() => this.Name);
        }
    }
}

The XAML is just a ListBox in the MainView and a TextBlock in the ItemView:

<Window x:Class="CaliburnMicroTest2.Views.MainView"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainView" Height="300" Width="300">
    <Grid>
        <ListBox x:Name="Items"/>
    </Grid>
</Window>

<UserControl x:Class="CaliburnMicroTest2.Views.ItemView"
             xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml">
    <TextBlock x:Name="Name"/>
</UserControl>

Anyone had this happen before?

seems like an awful amount of viewmodels to be discovered / containered at once.  I couldn't imagine the memory foot print that application with that many viewmodels would require if the complexity of the viewmodels was exponentially higher than 1 property.

Memory isn't really a concern here, as the ViewModels (or the underlying models) don't hold much data. Think of simple Customer records or similar... you can easily fit a few thousand of those into a couple of megabytes. However, these VMs pull data from the web and I would have liked to use the Activation/Deactivation capabilities of Caliburn's Conductors and Screens for that instead of rolling my own system. With that unfortunate StackOverflowException this isn't feasible, though.

In any case this seems like a bug. If there is a known limit for the number of usable items in a conductor, then that should be documented and probably coded against. I'd rather like to get a TooManyItemsException or similar instead of a crashing application upon exit. Especially since the application actually works just as expected... until you try to close it, at least.

Have you tried debugging the TryClose method? Seems like a recursion issue. Where does the exception is raised?

Had a look at it again and it seems DefaultCloseStrategy<T>.Evaluate(...) is the culprit.  It uses recursion to iterate over the IEnumerator<T> and evaluates, if all items are ready to be closed. Seems strange to use recursion there - why not just straightforward iterate over the IEnumerable<T> (e.g. using foreach)?

Alright, I just wrote my custom close strategy and resolved the recursion. That's what I ended up with:

public class CustomCloseStrategy<T> : ICloseStrategy<T>
{   
    List<T> closable;
    bool finalResult;
    readonly bool closeConductedItemsWhenConductorCannotClose;

    /// <summary>
    /// Creates an instance of the class.
    /// </summary>
    /// <param name="closeConductedItemsWhenConductorCannotClose">Indicates that even if all conducted items are not closable, those that are should be closed. The default is FALSE.</param>
    public CustomCloseStrategy(bool closeConductedItemsWhenConductorCannotClose = false)
    {
        this.closeConductedItemsWhenConductorCannotClose = closeConductedItemsWhenConductorCannotClose;
    }

    /// <summary>
    /// Executes the strategy.
    /// </summary>
    /// <param name="toClose">Items that are requesting close.</param>
    /// <param name="callback">The action to call when all enumeration is complete and the close results are aggregated.
    /// The bool indicates whether close can occur. The enumerable indicates which children should close if the parent cannot.</param>
    public void Execute(IEnumerable<T> toClose, Action<bool, IEnumerable<T>> callback) {
        finalResult = true;
        closable = new List<T>();

        foreach (var current in toClose)
        {
            var guard = current as IGuardClose;
            if (guard != null)
            {
                guard.CanClose(canClose =>
                {
                    if (canClose)
                    {
                        closable.Add(current);
                    }

                    finalResult = finalResult && canClose;
                });
            }
            else
            {
                closable.Add(current);
            }
        }

        callback(finalResult, closeConductedItemsWhenConductorCannotClose ? closable : new List<T>());
        closable = null;
    }
}

Since there was no need for it anymore, I removed the Evaluate(..) method and included all of its logic directly in the Execute(..) method. It should behave exactly the same, I think, but avoids the recursion and doesn't cause the stack overflow. In my quick testing after replacing the Conductor's CloseStrategy with my new CustomCloseStrategy it seems to work perfectly fine and my app doesn't crash anymore. This should probably be fixed in the Caliburn Micro source.

It's probably not the regular use case to have thousands of items being managed by a Conductor, but at first glance I don't see a technical reason for this recursion anyway.

I tend to agree. Using an iterative solution is definitely better, whenever possible.

Recursion has its place, but when you just iterate over a collection from start to finish, it's an odd choice.

Using recursion is not obvious here:

Think of a guard that's processing is defered (e.g. a Messagbox waiting for user input).
The recursive solution only continues to check the next item in the enumeration if the previous is processed (in clase of MessageBox when the user clicks a button).
When the last item finished the CanClose check, the callback is executed (not earlier).

Maybe we can figure out an iterative solution, but it will not be as simple as shown in this discussion.

I don't think I follow you there. The code in DefaultCloseStrategy.cs simply gets the IEnumerator<T> and calls MoveNext() on it until there are no more items being returned. Doesn't "foreach" do exactly the same under the hood? I didn't make any other changes to the original code - all enumerated items are being processed in the exact same way and the exact same order and the callback is only getting called after all items have been processed and Enumerator has stopped returning new items.

... and if I'm wrong about foreach, you can also just replace the construct with manual calls to GetEnumerator(), MoveNext() and Current and achieve the same result as the recursive calls.

In any case, the default recursive solution is broken for my use case as it simply crashes the app. Thankfully Caliburn Micro is more than flexible enough to plugin a solution that works for me, so I won't complain about that. I just still don't see the reasoning behind using recursion to iterate over an enumerable collection, especially if that method may crash the whole application at some point.