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

LifetimeScopeEnding event fired in a way that objects cannot finish their work gracefully #828

Closed
MT-PL-SWF-Dev opened this issue Feb 1, 2017 · 3 comments

Comments

@MT-PL-SWF-Dev
Copy link

MT-PL-SWF-Dev commented Feb 1, 2017

We are encountering an issue that our application logs are (not always) cluttered with misleading errors (ObjectDisposedException) that arise from the way LifetimeScope disposal is implemented.
We could work around that issue on the application level, but it would require additional code that wouldn't have to be implemented if the LifetimeScopeEnding event was working a bit differently.

My case is about a worker executing a task in a loop - the task requires to resolve some objects from the LifetimeScope and process them. At first glance it might seem similar to this bug but it is not - the task needs to create child LifetimeScopes per the item (if you are interested in the business case I can provide you further details, but for the sake of simplicity I'm ommitting them here).
Below I present (in pseudo-code) how the current (version 4.3.0.0) disposal of LifetimeScope looks like (combination Disposable.Dispose() and LifetimeScope.Dispose(bool)):

  1. EnsureNotDisposedOtherwiseReturn
  2. MarkAsDisposed
  3. RaiseCurrentScopeEndingEvent
  4. DisposeItems
  5. GC.SuppressFinalize

That's not entirely true, because step 1 and 2 are done together (Interlock.Exchange) but I need to refer to point 2 explicitly.
The problem comes because step 3 is occurring after the step 2 - the LifetimeScope is already marked as disposed (hence it will not resolve any further objects) BEFORE the interested objects are informed, that they should prepare for the LifetimeScope disposal.
From my perspective this event is neither LifetimeScopeEnding (before actually blocking the possibility to resolve objects) nor LifetimeScopeEnded (which would appear after full cleanup) - it is something in between.

Having wrote that I think it still might not be 100% clear what is the need, therefore I provide a unit test below. But before that, please bear in mind several points:

  • the unit test is about the container itself (so RootLifetimeScope) but it is the same for every LifetimeScope
  • the _synchronizationResetEvent is in place to enable the race condition to happen (it is not in the production code) as well as to make the test machine-independent (instead of adding explicit Thread.Sleep())
public class SomeWorker : IDisposable
{
	private readonly AutoResetEvent _synchronizationResetEvent = new AutoResetEvent(false);
	private readonly AutoResetEvent _disposingResetEvent = new AutoResetEvent(false);
	private readonly ILifetimeScope _lifetimeScope;
	private readonly Thread _workingThread;

	public SomeWorker(ILifetimeScope lifetimeScope)
	{
		_lifetimeScope = lifetimeScope;
		_lifetimeScope.CurrentScopeEnding += StopWork;

		_workingThread = new Thread(DoWork) {IsBackground = true};
		_workingThread.Start();
	}

	public void Dispose()
	{
		_synchronizationResetEvent.Dispose();
		_disposingResetEvent.Dispose();
	}

	private void DoWork()
	{
		var waitHandles = new WaitHandle[] {_disposingResetEvent, _synchronizationResetEvent};
		while (WaitHandle.WaitAny(waitHandles) != 0)
		{
			_lifetimeScope.Resolve<SomeWorker>();
		}
	}

	private void StopWork(object sender, LifetimeScopeEndingEventArgs e)
	{
		e.LifetimeScope.CurrentScopeEnding -= StopWork;
		_synchronizationResetEvent.Set();
		_disposingResetEvent.Set();
		_workingThread.Join();
	}
}

[TestMethod]
public void LifetimeScope_Disposing_ObjectsAreInformedPriorToActualDisposalInOrderToGracefullyFinishTheirWork()
{
	var containerBuilder = new ContainerBuilder();
	containerBuilder.RegisterType<SomeWorker>().AsSelf().SingleInstance();
	using (var container = containerBuilder.Build())
	{
		container.Resolve<SomeWorker>();
	}
}

At first the cleanup code was in the dispose method - which I found not a good place to do business logic cleanup and therefore ended up with using the event Autofac provides.
As I said in the beginning - we could work around this behavior with our custom lifetime events instead of depending on the LifetimeScopeEnding event. But I would still expect that you could gracefully cleanup your work in this event before the LifetimeScope is becoming disposed the same way as you can start using IStartables immediately after they are resolved (you don't need to add workarounds for them).
As for how to implement it in the code - this is a bit more complex, because Disposable object is used quite frequently in the library and the fix is about making Dispose() method looking a bit differently for only one class, but it still cane be done in a nice way.
Thanks,
Patryk

@tillig
Copy link
Member

tillig commented Feb 1, 2017

This is a little heavy to wade through. Here's what I gather:

  • You have multiple threads sharing a lifetime scope instance.
  • One thread is trying to resolve something while another thread is disposing the scope.
  • This causes a problem if you try to use the scope disposal event as a trigger for handling other class cleanup work because the scope gets marked as disposed before the event gets raised.

Is that a correct summary of the issue?

@MT-PL-SWF-Dev
Copy link
Author

Yes, that's a good summary.
There is no built-in mechanism on which the thread could rely on to gracefully finish its work. In terms of time boundaries between which you are safe to resolve objects from the lifetime scope you can do it from just after ChildLifetimeScopeBeginning and until some time before CurrentScopeEnding. So the latter boundary is not well defined.
In the code above, if you commented out the _synchronizationResetEvent.Set(); you'd see that the test pases. This line is exactly what's needed to invoke the race condition.
And as I stated earlier - I could fire my own "ScopeAboutToBeEnded" event, but for me it's a pity, that the closing boundary (as described above) is not as reliable as the opening one.

@tillig
Copy link
Member

tillig commented Sep 24, 2020

We're a good two years into this issue and we both don't have any community votes on the issue and we don't have a good repro on how to test it. It appears that, at least in today's code (v6 in develop) the event is raised before the lifetime scope is disposed, so I think it should work the way it needs to.

If not, we can open reopen the issue if we can get maybe a failing unit test or something to illustrate what's not working such that we can fix it.

@tillig tillig closed this as completed Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants