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.

Sign up for GitHub

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

Fix NRE when default value is null and ServiceCallSite is not found #44877

Merged
merged 5 commits into from
Nov 25, 2020
Merged

Fix NRE when default value is null and ServiceCallSite is not found #44877

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
229f15e
Fix NRE when default value is null and ServiceCallSite is not found (…
maryamariyan Nov 18, 2020
69422ce
packaging changes
maryamariyan Nov 21, 2020
65df681
correction
maryamariyan Nov 21, 2020
46cbf57
Feedback changes
maryamariyan Nov 24, 2020
e95e58b
Merge branch 'release/5.0' into port-nre
Anipik Nov 25, 2020
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
8 changes: 8 additions & 0 deletions src/libraries/Microsoft.Extensions.DependencyInjection/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<Import Project="..\Directory.Build.props" />
<PropertyGroup>
<StrongNameKeyId>Open</StrongNameKeyId>
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
<PackageVersion>5.0.1</PackageVersion>
<AssemblyVersion>5.0.0.1</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

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

@dougbu @Anipik remind me what our plan is for references shared in the transport package? Did we agree to freeze the assembly version or no? If so, then we should move the AssemblyVersion change to the src project.

Copy link
Member

Choose a reason for hiding this comment

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

@ericstj we agreed dotnet/runtime shouldn't create the transport package unless we really need to service the targeting packs. I don't remember if @Pilchie's document specifically covers $(AssemblyVersion) for ref/ assemblies but do recall it recommends not changing $(AssemblyVersion) for anything but Framework assemblies in servicing.

Copy link
Member

Choose a reason for hiding this comment

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

I believe in that document we concluded that assemblyVersion should change only apply to the net4x targetframeworks when an assembly overlaps the shared framework. We could follow that rule here. @Anipik did you have anything in the servicing doc about this with respect to ensuring servicable ref packs?

Copy link
Contributor

Choose a reason for hiding this comment

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

we concluded that assemblyVersion should change only apply to the net4x targetframeworks when an assembly overlaps the shared framework

yes we increment if we have any asset that applies netfx or netstandard, following the same thing here.

@Anipik did you have anything in the servicing doc about this with respect to ensuring servicable ref packs?

sadly nothing in the servicing doc with respect to ref packs. We need to add few more templates as well, i can add ref pack specific guidelines with that

Copy link
Member

Choose a reason for hiding this comment

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

we concluded that assemblyVersion should change only apply to the net4x targetframeworks when an assembly overlaps the shared framework

yes we increment if we have any asset that applies netfx or netstandard, following the same thing here.

@Anipik this doesn't seem to match the condition @ericstj described. NetStandard assemblies from projects that end up in the shared framework shouldn't vary because it ties external libraries to specific shared framework versions.

The exact Condition for stable versions (overriding Arcade's defaults) in dotnet/aspnetcore is '$(IsAspNetCoreApp)' == 'true' AND '$(Language)' == 'C#' AND '$(TargetFrameworkIdentifier)' != '.NETFramework'. This (plus targeting or multi-targeting net5.0 in those projects) means all assemblies in Microsoft.AspNetCore.App from our repo have stable versions and the same is true of separately-shipped NetStandard assemblies from those projects. Everything else varies in servicing.

Crucially this is not what dotnet/runtime is doing for assemblies that end up in Microsoft.AspNetCore.App. So far, Microsoft.Extensions.DependencyInjection and System.IO.Pipelines have new assembly versions. I needed to workaround the first change and revert bfc1ec679202 / #25851 (to handle the second) in #28087. bfc1ec679202 was our change in expectation of stable assembly versions from dotnet/runtime.

</PropertyGroup>
</Project>
6 changes: 4 additions & 2 deletions src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ConstantCallSite.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
{
internal class ConstantCallSite : ServiceCallSite
{
private readonly Type _serviceType;
internal object DefaultValue { get; }

public ConstantCallSite(Type serviceType, object defaultValue): base(ResultCache.None)
{
_serviceType = serviceType ?? throw new ArgumentNullException(nameof(serviceType));
if (defaultValue != null && !serviceType.IsInstanceOfType(defaultValue))
{
throw new ArgumentException(SR.Format(SR.ConstantCantBeConvertedToServiceType, defaultValue.GetType(), serviceType));
Expand All @@ -19,8 +21,8 @@ public ConstantCallSite(Type serviceType, object defaultValue): base(ResultCache
DefaultValue = defaultValue;
}

public override Type ServiceType => DefaultValue.GetType();
public override Type ImplementationType => DefaultValue.GetType();
public override Type ServiceType => DefaultValue?.GetType() ?? _serviceType;
public override Type ImplementationType => DefaultValue?.GetType() ?? _serviceType;
public override CallSiteKind Kind { get; } = CallSiteKind.Constant;
}
}
31 changes: 31 additions & 0 deletions src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,37 @@ public void DoesNotThrowWhenServiceIsUsedAsEnumerableAndNotInOneCallSite()
Assert.NotNull(compileCallSite);
}

[Theory]
[InlineData(ServiceProviderMode.Default)]
[InlineData(ServiceProviderMode.Dynamic)]
[InlineData(ServiceProviderMode.Runtime)]
[InlineData(ServiceProviderMode.Expressions)]
[InlineData(ServiceProviderMode.ILEmit)]
private void NoServiceCallsite_DefaultValueNull_DoesNotThrow(ServiceProviderMode mode)
{
var descriptors = new ServiceCollection();
descriptors.AddTransient<ServiceG>();

var provider = descriptors.BuildServiceProvider(mode);
ServiceF instance = ActivatorUtilities.CreateInstance<ServiceF>(provider);

Assert.NotNull(instance);
}

private interface IServiceG
{
}

private class ServiceG
{
public ServiceG(IServiceG service = null) { }
}

private class ServiceF
{
public ServiceF(ServiceG service) { }
}

private class ServiceD
{
public ServiceD(IEnumerable<ServiceA> services)
Expand Down
1 change: 1 addition & 0 deletions ...Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderCompilationTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace Microsoft.Extensions.DependencyInjection.Tests
public class ServiceProviderCompilationTest
{
[Theory]
[InlineData(ServiceProviderMode.Default, typeof(I999))]
[InlineData(ServiceProviderMode.Dynamic, typeof(I999))]
[InlineData(ServiceProviderMode.Runtime, typeof(I999))]
[InlineData(ServiceProviderMode.ILEmit, typeof(I999))]
Expand Down
1 change: 1 addition & 0 deletions src/libraries/libraries-packages.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<ProjectReference Include="$(MSBuildThisFileDirectory)*\pkg\**\*.pkgproj" Condition="('$(BuildAllConfigurations)' == 'true' or '$(DotNetBuildFromSource)' == 'true') And '$(BuildAllOOBPackages)' == 'true'" />
<!-- If setting BuildAllOOBPackages to false, add bellow the individual OOB packages you want to continue to build -->
<ProjectReference Include="$(LibrariesProjectRoot)\System.IO.Pipelines\pkg\System.IO.Pipelines.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
<ProjectReference Include="$(LibrariesProjectRoot)\Microsoft.Extensions.DependencyInjection\pkg\Microsoft.Extensions.DependencyInjection.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
</ItemGroup>

<!-- Need the PackageIndexFile file property from baseline.props -->
Expand Down
13 changes: 11 additions & 2 deletions src/libraries/pkg/baseline/packageIndex.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,11 +507,20 @@
"3.1.0",
"3.1.1",
"3.1.2",
"5.0.0"
"5.0.0",
"3.1.3",
"3.1.4",
"3.1.5",
"3.1.6",
"3.1.7",
"3.1.8",
"3.1.9",
"3.1.10"
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
],
"InboxOn": {},
"AssemblyVersionInPackageVersion": {
"5.0.0.0": "5.0.0"
"5.0.0.0": "5.0.0",
"5.0.0.1": "5.0.1"
}
},
"Microsoft.Extensions.DependencyInjection.Abstractions": {
Expand Down