Bug when injecting IEnumerable<T> (or any collection type)

[Aaron Lerch]

I just submitted this issue to CodePlex:
http://ninject.codeplex.com/WorkItem/View.aspx?WorkItemId=2948

I'll paste the contents here, as it's hard to explain and I put a lot
of time into writing up that explanation. :)
I appreciate any thoughts on the bug, or the possible resolutions. I'm
happy to submit a patch (or whatever the git version of a patch is
called), but it's a pretty trivial fix I think, so maybe that's not
even needed.
I spent about 8 hours going between Ninject code, my code, and a bunch
of "WTF"'s. :) I couldn't reproduce it in a test app forever until I
finally realized that it took *3 or more* extra LINQ queries to be
executed, and I was just executing TWO in my test code. That'll make
sense once you read the description below (hopefully).

Aaron

------------------------------------
Okay, bear with me on this one, it's hard to explain clearly but I'll
try my best.

In the following scenario:
1. Multiple bindings to the same interface exist
2. The bindings are not transient (they are cached somehow, whether
it's per-request or singleton or other)
3. There are no cyclical dependencies
4. A class is injected with an IEnumerable<T> of that type
5. The class executes LINQ queries on that injected enumerable to, for
example, separate it out into "categories" of instances

Then: an ActivationException is thrown stating that cyclical
dependencies are detected.

Cause:
Because Ninject uses LINQ to build up the resolved types, it is delay
executed. When I execute "more LINQ" on that same list and then
finally reference it, the LINQ infrastructure ends up executing the
original LINQ query multiple times over the entire list. This isn't
bad, except that an execution path inside
Ninject.Activation.Context.Resolve() can return the resolved instance
(if it was cached) without popping the requested binding off the
"ActiveBindings" stack.
It takes 3 or more "sub-queries" to make this happen because:
1. First time through everything resolves correctly, instances are
created and put into the cache, and the "ActiveBindings" stack is
pushed/popped correctly.
2. Second time through, the instances are pulled out of the cache and
the ActiveBindings stack is left in a bad state (not popped
correctly), but things still return correctly.
3. Third (and more) time through the sanity check against the
"ActiveBindings" stack fails (because of the previous iteration's bug)
and the exception is thrown.

Workaround:
Instead of executing further LINQ queries on the IEnumerable<T> given
to me by Ninject, I can call ".ToList()" on it to execute the LINQ,
and then do further queries on *that* list. By then, the Ninject LINQ
only executes once and resolves all the types correctly only once.

Possible solutions (need advice from those more familiar with the
internals of Ninject):
Choice 1:
In /Activation/Context.cs, the "public object Resolve()" method could
be changed from
if (reference.Instance != null)
return reference.Instance

to

if (reference.Instance != null)
{
Request.ActiveBindings.Pop();
return reference.Instance;
}

This would pop the binding off the "ActiveBindings" stack when it was
returned from the cache, and would prevent the stack from getting into
a bad state.
Note that this solution wouldn't address or fix the "side note" I have
at the bottom of this report.

Choice 2:
Before Ninject injects the IEnumerable<T>, IList<T>, or
ICollection<T>, it could "execute" the results of the LINQ query, so
the incoming values to the class would be resolved at that moment and
not wait until I used them within the class itself, if I did more LINQ
querying on the list. It's potentially *slightly* slower this way, but
the net gain would be worth it, I think. Especially given that this
would also resolve the "side note" below.

Choice 3:
Do both 1 and 2. :) This would probably clean it up the most, fixing
both "bugs".

Example code for reproduction is attached. Create a new console app
and use the file attached.

SIDE NOTE:
A side note, if I declare the bindings as transient, then this works,
but only because they are never cached. Which also means that for a
given input enumerable it will create a new instance of the class each
time I execute a LINQ query on it. So where I would normally expect
the incoming IEnumerable<T> to contain a set, it would actually create
potentially many extra instances.

[Nate Kohari]

Aaron:

This is an interesting issue. I think I understand the problem, but I'm
not sure I get why "choice 1" wouldn't eliminate it completely. When an
IEnumerable<T> is requested, Ninject intentionally delays the resolution
of the additional instances, allowing you to do something like:

kernel.GetAll<IService>().First();

or:

kernel.GetAll<IService>().Skip(2).Take(3);

It does appear that the ActiveBindings stack gets screwed up when the
instance is already cached, though.... but it seems like the change in
"choice 1" would fix the problem.


-Nate

[Aaron Lerch]

Yeah, that was my first thought as well, and indeed it would solve the
problem I reported.
The reason I suggested "choice 3" (which is both 1 and 2) was that it
would be more efficient for transient services.

If I was injected with an IEnumerable<T> containing a bunch of
services bound in the "transient" scope (the default) and I issued
more linq requests on that enumerable set (like you mentioned below),
the code as it stands would unnecessarily create multiple instances of
each transient type. Most would be thrown away, probably, but it seems
unnecessary that they're created.

Also, I haven't tested this to be sure, but I think that if I accepted
an IEnumerable<T> in my constructor (for example) and ran 2 different
linq queries over that set, and each query had conditions whereby the
same item was selected for the two different results, they would
actually be two different instances and not two references to the same
instance. That could be very confusing behavior. Edge case, though,
yes.

I can put together an example of this to verify it if that would help.

Overall, I care mostly about solving the original problem, but I just
wanted to point out that I think there might be other more subtle
issues for transient-scoped bindings.
Thanks!!

[Aaron Lerch]

Did you have any thoughts on this Nate? (or anybody else)
wouldn't this be a problem for transient services too?

[Aaron Lerch]

Has this been fixed? There haven't been any responses here, and the
codeplex issue hasn't been updated (and github's history didn't show
it as having been fixed, at least the last time I checked - github is
down right now).

Thanks!