Skip to content
This repository has been archived by the owner. It is now read-only.

DisposesInReverseOrderOfCreation Specification Test Expects Eager Enumerable #589

Closed
LordZoltan opened this issue Oct 5, 2017 · 3 comments
Closed
Assignees

Comments

@LordZoltan
Copy link

@LordZoltan LordZoltan commented Oct 5, 2017

I'm updating the Rezolver IOC Container to implement the 2.0 DependencyInjection spec (1.1 implementation is currently stable) and the test DisposesInReverseOrderOfCreation is failing.

For completeness - this is the test from the tag rel/2.0.0 (the test is the same in the current version):

[Fact]
public void DisposesInReverseOrderOfCreation()
{
// Arrange
var serviceCollection = new TestServiceCollection();
serviceCollection.AddSingleton<FakeDisposeCallback>();
serviceCollection.AddTransient<IFakeOuterService, FakeDisposableCallbackOuterService>();
serviceCollection.AddSingleton<IFakeMultipleService, FakeDisposableCallbackInnerService>();
serviceCollection.AddScoped<IFakeMultipleService, FakeDisposableCallbackInnerService>();
serviceCollection.AddTransient<IFakeMultipleService, FakeDisposableCallbackInnerService>();
serviceCollection.AddSingleton<IFakeService, FakeDisposableCallbackInnerService>();
var serviceProvider = CreateServiceProvider(serviceCollection);
var callback = serviceProvider.GetService<FakeDisposeCallback>();
var outer = serviceProvider.GetService<IFakeOuterService>();
// Act
((IDisposable)serviceProvider).Dispose();
// Assert
Assert.Equal(outer, callback.Disposed[0]);
Assert.Equal(outer.MultipleServices.Reverse(), callback.Disposed.Skip(1).Take(3).OfType<IFakeMultipleService>());
Assert.Equal(outer.SingleService, callback.Disposed[4]);
}

When I look at the stack trace of the failure - it's because my scope is failing to resolve an object because the scope has already been disposed:

Test Name:	DisposesInReverseOrderOfCreation
Test FullName:	Rezolver.Microsoft.Extensions.DependencyInjection.Tests.RezolverDISpecificationTests.DisposesInReverseOrderOfCreation (6a6e0c0ae894aabfd720e4ee377cae523966f445)
Test Source:	 : line 0
Test Outcome:	Failed
Test Duration:	0:00:00.032

Result StackTrace:	
at Rezolver.ContainerScope.Rezolver.IContainerScope.Resolve(IResolveContext context, ITarget target, Func`2 factory, ScopeBehaviour behaviour)
   at Rezolver.ResolveContextExtensions.Resolve(IResolveContext context, ITarget target, Func`2 factory, ScopeBehaviour behaviour)
   at lambda_method(Closure , IResolveContext )
   at Rezolver.Runtime.LazyEnumerable`1.<GetEnumerator>b__3_0(Func`2 f)
   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext()
   at System.Collections.Generic.EnumerableHelpers.ToArray[T](IEnumerable`1 source, Int32& length)
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.Enumerable.ReverseIterator`1.MoveNext()
   at Microsoft.Extensions.DependencyInjection.Specification.DependencyInjectionSpecificationTests.DisposesInReverseOrderOfCreation()
Result Message:	
System.ObjectDisposedException : This scope has been disposed
Object name: 'ContainerScope'.

On closer inspection I've realised it's because Rezolver creates lazy enumerables by default. This means that on line 681, the enumerable produced by the call outer.MultipleServices.Reverse() attempts to resolve new instances from the scope that has just been disposed of on line 677 when the Assert.Equal call tries to enumerate it to assert equality - a similar scenario to enumerating a set from an EF context after the context has been disposed.

Rezolver supports both eager and lazy-loaded enumerables (and can be conffigured for either per-type) - so I can change the implementation to switch to eager enumerables for its implementation of the MS DI abstraction, but it seems to me that this test is assuming a behaviour that is not specifically being tested - and, in the strictest sense, is unrealistic: if an IEnumerable<T> is all that's required, then FakeDisposableCallbackOuterService, and this test, should *not assume that the enumerable is backed by a fully-realised collection of objects - which is currently the only way to get a container implementation to pass this test.

Indeed, if that is indeed an expectation - say, in the way Asp.Net or other components use a DI Container via this abstraction, then there should be an explicit test which resolves an enumerable from the container and checks that all the expected instances are already created before enumeration takes place.

So I'm a bit lost at the minute - I see a couple of ways forward:

  • If the specification expects all container-resolved enumerables to be, in effect, arrays - then there should be a spec test to verify that; meaning that I can then force Rezolver to use eager enumerables for its MS DI implementation
  • If it doesn't expect that (or, more importantly, is agnostic to it), then at the very least this test should be rewritten to make sure it passes for both lazy and eager enumerables

Thanks in advance.

@LordZoltan LordZoltan changed the title DisposesInReverseOrderOfCreation Specification Test Misuses IEnumerable DisposesInReverseOrderOfCreation Specification Test Expects Eager Enumerable Oct 6, 2017
@pakrym pakrym self-assigned this Oct 6, 2017
@pakrym
Copy link
Member

@pakrym pakrym commented Oct 6, 2017

Good point, will fix.

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Oct 6, 2017

Sounds reasonable

@LordZoltan
Copy link
Author

@LordZoltan LordZoltan commented Oct 6, 2017

Brilliant stuff, thanks :)

@pakrym pakrym closed this Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.