Fix NRE when default value is null and ServiceCallSite is not found #44877
Conversation
|
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue Details
|
|
@Anipik do we need to add the package changes and version to this? |
yes we require the entire thing, packageVersion, AssemblyVersion, & packageIndex Update, also need to add this package to libraries-packages.proj |
ea74f2e to
d61f667
Compare
updated. |
…otnet#44675) * Fix NRE when default value is null and ServiceCallSite is not found
8743589 to
65df681
Compare
src/libraries/Microsoft.Extensions.DependencyInjection/Directory.Build.props
Outdated
Show resolved
Hide resolved
|
Hello @maryamariyan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
JIT failure is #44831 |
| <Import Project="..\Directory.Build.props" /> | ||
| <PropertyGroup> | ||
| <PackageVersion>5.0.1</PackageVersion> | ||
| <AssemblyVersion>5.0.0.1</AssemblyVersion> |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Backport of #44675 to release/5.0
/cc @eerhardt
Customer Impact
Scenario: When trying to resolve a transient service expecting a (non-registered) service with a default value set (of null).
When this issue hits, the apps get a first chance exception (NRE) in the background and would then end up using a worse performing reflection rather than Ref.Emit code on the transient services.
Testing
Using the regression test that is part of this PR.
Risk
Low - Fixing an NRE to return the Type of the parameter when the default value is
null.Regression from 3.0?
Yes. From Pull Request #2501 · dotnet/extensions