Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,20 @@ static async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
// No further changes to _state.Disposables, are allowed.
_disposed = true;

// ResolvedServices is never cleared for singletons because there might be a compilation running in background
// trying to get a cached singleton service. If it doesn't find it
// it will try to create a new one which will result in an ObjectDisposedException.
}

return _disposables;
if (IsRootScope && !RootProvider.IsDisposed())
{
// If this ServiceProviderEngineScope instance is a root scope, disposing this instance will need to dispose the RootProvider too.
// Otherwise the RootProvider will never get disposed and will leak.
// Note, if the RootProvider get disposed first, it will automatically dispose all attached ServiceProviderEngineScope objects.
RootProvider.Dispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cant this just noop internally?

Copy link
Member Author

@tarekgh tarekgh Aug 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is simpler. no?

More code, more bugs :-) imagine in the future we add anything to the ServicePrivder dispose method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cant this just noop internally?

By "just noop internally", you mean adding the if check in ServiceProvider Dispose?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

}

// ResolvedServices is never cleared for singletons because there might be a compilation running in background
// trying to get a cached singleton service. If it doesn't find it
// it will try to create a new one which will result in an ObjectDisposedException.
return _disposables;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ internal ServiceProvider(ICollection<ServiceDescriptor> serviceDescriptors, Serv
/// <returns>The service that was produced.</returns>
public object? GetService(Type serviceType) => GetService(serviceType, Root);

internal bool IsDisposed() => _disposed;

/// <inheritdoc />
public void Dispose()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
using Xunit;

Expand All @@ -17,5 +18,16 @@ public void DoubleDisposeWorks()
serviceProviderEngineScope.Dispose();
serviceProviderEngineScope.Dispose();
}

[Fact]
public void RootEngineScopeDisposeTest()
{
var services = new ServiceCollection();
ServiceProvider sp = services.BuildServiceProvider();
var s = sp.GetRequiredService<IServiceProvider>();
((IDisposable)s).Dispose();

Assert.Throws<ObjectDisposedException>(() => sp.GetRequiredService<IServiceProvider>());
}
}
}