Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After Container.Dispose(), the container should throw ObjectDisposedException, not NullReferenceException #304

Closed

Conversation

asbjornu
Copy link
Contributor

I've not been able to reproduce this in a test, but given a rather complex plugin graph, I've discovered that resolving certain concrete, unregistered types, causes the SessionCache to throw a NullReferenceException.

This pull request tries to remedy this a bit by adding null-guards for all the objects that are involved in the given scenario. I've verified that these guards cause ArgumentNullException to be thrown instead, which is an improvement, but I've not been able to figure out the root of the problem yet.

The exception itself looks like this:

System.NullReferenceException occurred
  _HResult=-2147467261
  _message=Object reference not set to an instance of an object.
  HResult=-2147467261
  IsTransient=false
  Message=Object reference not set to an instance of an object.
  Source=StructureMap
  StackTrace:
       at StructureMap.SessionCache.GetDefault(Type pluginType, IPipelineGraph pipelineGraph) in c:\BuildAgent\work\996e173a8ceccdca\src\StructureMap\SessionCache.cs:line 43

After introducing the null-guards and catching the ArgumentNullException being thrown (with Debug > Exceptions in Visual Studio), I've managed to sew together this call stack:

StructureMap.SessionCache.GetDefault(System.Type pluginType, StructureMap.IPipelineGraph pipelineGraph):line 42
StructureMap.BuildSession.GetInstance(System.Type pluginType):line 60
StructureMap.Container.GetInstance(System.Type pluginType):line 331

I wish I had a test case and a fix, but the plugin graph this happens on is so complex that it will take time to reproduce it. In the meantime I hope leaving this here might give someone an idea. In any case, all unit tests still pass, so merging these null-guards should be safe and an improvement anyway.

@asbjornu
Copy link
Contributor Author

The NullReferenceException (ArgumentNullException with this PR merged) is being thrown when doing @Html.Partial("Partials/Search") within a Razor view, which invokes the StructureMapDependencyResolver.GetService(Type) method. Within this method, a nested container for the given request is found and the Type is being passed to the container's GetInstance(Type) method.

Since I haven't been able to reproduce the problem in a test yet, I assume the plugin graph is in some way to blame for the PipelineGraph passed to SessionCache.GetDefault() being null, but I haven't figured out why that is yet. When I inspect the Container, I can see that its _pipelineGraph field is indeed null. All of the ProfileName, Role and Model properties on the Container throw a NullReferenceException when being viewed in QuickWatch.

@asbjornu
Copy link
Contributor Author

Ah. I see now that Container.Dispose() sets _pipelineGraph to null. That must be it, I guess; that the nested container is disposed and I'm still trying to retrieve stuff from it. But instead of actually performing any more work and then failing because _pipelineGraph is null, shouldnt it throwObjectDisposedException? That would at least be a much more accurate and descriptive exception than theNullReferenceException` being thrown now.

…Arguments, String) if the container is disposed.
…icitArguments) if the container is disposed.
@asbjornu
Copy link
Contributor Author

I've added checks for _disposedLatch all over Container and if it's true, an ObjectDisposedException is thrown. All tests pass, but there should probably be more tests verifying the exception and there might be more places within the Container the check should be added to. I've only added them to methods that clearly involve _pluginGraph, but I think it makes sense that all methods and properties on a disposed object throws ObjectDisposedException.

@asbjornu asbjornu changed the title SessionCache throws NullReferenceException in a nested container After Container.Dispose(), the container should throw ObjectDisposedException, not NullReferenceException Dec 3, 2014
@jeremydmiller jeremydmiller modified the milestone: 3.1.5 Dec 12, 2014
@asbjornu
Copy link
Contributor Author

I'm thinking that the container perhaps should have a Disposed property that returns the value of _disposedLatch, so it's possible to figure out whether a Container is disposed or not without having to interact with it and catching ObjectDisposedException. Thoughts, @jeremydmiller?

Conflicts:
	src/StructureMap.Testing/Pipeline/NestedContainerSupportTester.cs
	src/StructureMap/Container.cs
	src/StructureMap/PluginGraphBuilder.cs
@asbjornu
Copy link
Contributor Author

Seems like this was fixed in 516d323. Closing.

@asbjornu asbjornu closed this Dec 16, 2015
@asbjornu asbjornu deleted the sessioncache-throws-nullref branch June 17, 2016 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants