Materialize IEnumerable<T> before injection

[BrunoJuchli]

Currently an IEnumerable which is injected will be "lazy" materialized when someone accesses it. This can lead to erratic behavior / deadlocks etc.

Please change it so that IEnumerable is backed by a materialized collection which is "eager" materialized before injection.

[idavis]

If I recall correctly, if you inject an IList or an array this is taken care of.

-Ian

On Mon, Apr 14, 2014 at 6:18 AM, Bruno Juchli notifications@github.comwrote:

Currently the IEnumerable which is injected will be "lazy" materialized when someone accesses it. This can lead to erratic behavior / deadlocks etc.

Please change it so that IEnumerable is backed by a materialized collection which is "eager" materialized before injection.

— Reply to this email directly or view it on GitHubhttps://github.com/ninject/ninject/issues/128 .

[BrunoJuchli]

Yes i would agree. Still this is a pitfall for bugs. Also using collections as a workaround is a tiny bit smelly: i don't ever want to add or remove items to / from that collection.

(we (Erowa AG / Marco Erni,...) have also already had a discussion about this with @remogloor).

Maybe support for IReadOnlyCollection<T> could also be added to ninject?

[danielmarbach]

The problem is that way you are esentially redefining the semantics of IEnumerable! An IEnumerable is only evaluated when iterated. The currwnt behavior maked total sense from a usage perspectiv

Am 14.04.2014 um 16:08 schrieb Bruno Juchli notifications@github.com:

Yes i would agree. Still this is a pitfall for bugs. Also using collectionis as a workaround is a tiny bit smelly: i don't ever want to add or remove items to / from that collection.

(we (Erowa AG / Marco Erni,...) have also already had a discussion about this with @remogloor).

Maybe support for IReadOnlyCollection could also be added to ninject?

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

[idavis]

I am in agreement with @danielmarbach. The usage/point of IEnumerable is based on lazy evaluation. IReadonlyCollection<T> is one option, but an array works just as well and you can initialize your collection from the array.

[BrunoJuchli]

@danielmarbach Actually neither MSDN (http://msdn.microsoft.com/en-us/library/system.collections.ienumerable.aspx) nor Wikipedia (http://en.wikipedia.org/wiki/Iterator), nor often-read Stackoverflow entries (http://stackoverflow.com/questions/716238/distinction-between-iterator-and-enumerator) say anything about when the "collection" is built. Yours is probably an assumption originating from broad use of LINQ.

Also i did not say that the IEnumerable should be evaluated before injection, i only stated that the backing collection (or elements, for that matter), should be instanciated beforehand.

In any case, one could still use Lazy<IEnumerable<T>> if one would want to have lazy creation of the objects.

I still stress that the current implementation of IEnumerable<T> support is dangerous. Would you like to discuss / debate that?

In any case, i would agree that lots of people associate IEnumerable with late (lazy) creation (eventhough there is no guarantee made that it is actually done lazily). Since this may also lead to confusion, an acceptable solution may be to remove IEnumerable<T> support and introduce IReadOnlyCollection<T> and Lazy<IReadOnlyCollection<T>> support instead.

@idavis I like your array suggestion. There's a (smallish) caveat: Array.Add(...) method renders an array mutable. Also array explicitly implement the IList<T>.Add(...) method.

(I'll try to discuss it with the team in the few remaining days I'm spending with them ;-)).

Summary IMO:

• IEnumerable<T> does not make any statements about when the collection is materialized.
• current IEnumerable<T> support is a pitfall, cause of bugs, presents another entry threshold for new users and programming beginners
• it is highly doubtable that there is any (broad) use case for injecting modifiable collections. It contradicts general rules (..,ISP,..) and thus makes usage of ninject harder. And also is slightly smelly.
• IReadonlyCollection<T> is only available from .net 4.5 on and thus a backwards-compatibility issue as long as .net < 4.5 is supported. (what is the outlook for ninject 4?)
• Current IEnumerable<T> behavior could, if one would dare to wish for it, be offered by Lazy<IEnumerable<T>>, which does make an explicit statement about materialization (=> not instanciated before you are accessing Lazy.Value).

Also, there's the point of funding of development work required:

• We already discussed this with Remo. EROWA is going to fund some of the change requests we've placed / are going to place. I'm currently in doubt whether this is one of them but i'll check.

Addendum: i don't dispute that there may be (most likely are) significant issues with what i propose in regards of breaking the interface, backwards compatibility. I think we should discuss these, too.

[marcoerni]

As discussed with @remogloor, lazy resolving of the enumerable can have bad side effects according to scopes and locking of them. In fact when you let inject an enumerable you always have to materialize the enumerable in the constructor using .ToList(), ToArray() or something similar. Materializing the enumerable during run-time can produce unexpected exceptions.

[BrunoJuchli]

Link to issue "reports":

http://ninject.codeplex.com/workitem/2948 https://groups.google.com/forum/#!topic/ninject/5WiOkJIImDE

Somewhat related: https://groups.google.com/forum/#!topic/ninject/f2HgHwTRFwY

[brandondahler]

+1, my application requires me to resolve a db context factory based on the url that the site is accessed from (basic multi-tenant application with separate databases per site).

When using async/await with this setup, the dependency resolution gets bumped to a ThreadPool thread and causes the factory to fail to be created. Basic pseudocode of the issue:

// DbContext
class DbContext
{
    ...
}

interface IDbContextFactory
{
    DbContext Create();
}

class DefaultDbContextFactory
{
    Func<DbContext> lazyFactory;

    DefaultDbContextFactory()
    {
        lazyFactory =  SomethingThatUsesHttpContextToProduceLazyFactory()
    }

    DbContext Create()
    {
        return lazyFactory();
     }
}

// Providers
interface IProvider
{
    Task<Something> ProduceSomethingAsync();
}

class ProviderA
{
    public ProviderA(IDbContextFactory contextFactory)
    {
        ...
     }

    Task<Something> ProduceSomethingAsync()
    {
        ...
     }
}

class ProviderB
{
    public ProviderB(IDbContextFactory contextFactory)
    {
        ...
     }

    Task<Something> ProduceSomethingAsync()
    {
        ...
     }
}

// Resolver
interface IResolver
{
    Task<Something> Resolve();
}

class DefaultResolver
{
    IEnumerable<IProvider> _providers;
    public DefaultResolver(IEnumerable<IProvider> providers)
    {
        _providers = providers;
    }

    public Task<Something> Resolve()
    {
         // Resolves IDbContextFactory dependencies at each enumeration
         // Would throw on second item due to HttpContext.Current == null
         foreach (provdier in _providers)
         {

          }
     }
}

Obviously I'll fix this myself by changing the DefaultResolver constructor to:

    public DefaultResolver(IEnumerable<IProvider> providers)
    {
        // Resolves all IDbContextFactory dependencies now
        _providers = providers.ToArray();
    }

[BrunoJuchli]

@danielmarbach In reference to your answer on #252: Sorry for being so confrontative. I concur you never disagreed with the reasoning that the current behavior of lazy-materialization is dangerous. I also concur that the original implementation does not violate the semantics of IEnumerable - no beef there.

However, your answer insinuated that you were categorically against the proposed change. And the reasoning you provided, that's what I don't agree with and where I still believe that you're wrong.

Yesterday you wrote:

(...) Only when the enumerator is accessed by design the enumeration and thus first materialization should happen. (...)

(...)But if you look at the larger .NET ecosystem and what the history and introduction of LINQ taught us is that enumerables cannot be assumed materialized thus it is a perfectly valid implemenation that ninject decided not to.(...)

I fully agree with the second part - enumerables cannot be assumed materialized. That is part of the semantics, yes. Might have been so before LINQ, even.

However, I also postulate that the semantics here include the reversal of the sentence:

Enumerables cannot be assumed unmaterialized

Simply, the semantics probably don't include anything about materialization. Now, to support my arguments:

From MSDN: Notice, IEnumerable exists since in .net 1.1, while LINQ was introduced in .NET 3.5.

Notice, ICollection inherits IEnumerable.

This means, there's tons of implementations and usages of materialized lists in the BCL where enumerables are indeed materialized, I think that's already more than enough proof for:

Enumerables cannot be assumed unmaterialized

and consequently that you statement:

(...) you are esentially redefining the semantics of IEnumerable (...)

is wrong.

Breaking Change

Of course, the change being a breaking change is a very important factor and what the discussion should be about, effectively.