Desktop OOB System.Net.Http - don't ship desktop implementation

[karelz]

With netstandard2.0.dll we will facade the OOB into Desktop .NET.
Is it something we should do also in the standalone System.Net.Http NuGet package? i.e. release yet another version which entirely removes the implementation in CoreFX and ships just the facade on Desktop?

@weshaggard thoughts?

[davidsh]

If we do this and keep the newer APIs that were added to NETStandard20, then we will have a ref/def mismatch. The ref for System.Net.Http will have 8 new APIs but the implementation (which will be a facade on Desktop) won't have those members. So, we will get "MissingMethodException". Is this what we want even when .NET Core 2.0 NuGet package of System.Net.Http directly?

[karelz]

I think it is desirable to have the same experience with .NET Standard 2.0 and direct targeting of .NET Core 2.0. I would prefer to do a clean cut with 2.0 and not guide people into using package we do not test in the environment it ships in (on Desktop).
Whoever truly needs those 8 APIs can stay on older package version or can use the workarounds we proposed.
It will also get rid of the binding redirects - which are pain to maintain (see #17770 and also in #18280).

If we want to keep it, we would have to stand up test infrastructure testing the combo -- probably ourselves as infra team is quite busy. So it comes back to cost/benefit ratio. Cost is non-trivial (medium to high). Value is ... small IMO.
I am more than happy to have larger discussion on networking v-team, but so far I don't see any reason for keeping the package. If there are consequences I am not aware of, I'll be happy to change my position.

[weshaggard]

@davidsh I assume dotnet/standard@622e5b3#diff-f0db2de692a1b3bca91f4d42eab8a297R75 are the APIs you are referring to correct? We could make a decision to remove those from .NET Standard 2.0 but we have similar APIs (as you can see from the rest of that commit) that are in a similar boat.

I tend to agree with @karelz here we should create a netstandard2.0 configuration for this package that just type-forwards to netstandard.dll.

[davidsh]

@davidsh I assume dotnet/standard@622e5b3#diff-f0db2de692a1b3bca91f4d42eab8a297R75 are the APIs you are referring to correct?

Yes, I am talking about the 8 new properties added to the HttpClientHandler class (part of System.Net.Http)

[davidsh]

@weshaggard @karelz How do we move forward on making a decision here? If we remove these APIs from NETStandard2.0, then that will break people that have started using them.

[weshaggard]

then that will break people that have started using them.

We aren't really worried about folks using them from netstandard2.0 itself but folks consuming the netstandard1.6 version are the set to worry about. Do we have any data about the usages of them? I'd be OK keeping them in netstandard2.0 but they would still throw on .NET Framework.

[karelz]

We never discussed removing the APIs from .NET Standard 2.0 - that would be compilation breaking change.
Desktop will eventually catch up and it already misses some ~100 APIs which will throw at runtime. We are "just" adding 8 more. Not ideal, but best solution under the circumstances.

I thought that we had lengthy discussions and email threads which clarify that by now. Maybe we should summarize it here ... would that be helpful?

[davidsh]

Ok. Then I don't understand what we are proposing to do here for this issue. If we remove the 'net46' binary implementation from this System.Net.Http package, then are we still keeping the 8 new APIs in the 'ref' folder for the package even when we remove the implementation itself?

Or, are we updating the 'ref' itself as well to match EXACTLY 'net46' which means it won't have the 8 new APIs.

[karelz]

What is the 'ref' for? For targeting net46 profile? Or also for targeting .NET Core, etc.?

[davidsh]

There is only one 'ref' file for all platforms.

https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/ref/System.Net.Http.cs

[karelz]

But it tends to have ifdefs per platform -- the new 8 APIs should be ifdefed out of net46 IMO. I'd wait on @weshaggard's guidance. I am just guessing based on partial info.

[davidsh]

But it tends to have ifdefs per platform -- the new 8 APIs should be ifdefed out of net46 IMO.

We have never done that in the 'ref'.

[karelz]

It seems that 'ref' is only for .NET Core targeting (example of .NET Core-only API adiditon). Maybe packages which target also Desktop compile ref twice? @weshaggard will know ;-)

[weshaggard]

cc @ericstj so he can have an opinion on this as well.

@karelz I do think we should explicitly list what we had decided because I honestly don't remember and we jumped around so much I don't know exactly where everything landed.

If we decide to stop shipping the OOB in a future version it will be a breaking change and assuming we accept that then we don't need to ship anything ref or lib for net46 in the new package as it will just get picked up from the .NET Framework itself. When folks target netfx directly they will not see the 8 new APIs but if they target netstandard they will see the new APIs, but they will just throw if they use them and try to run on netfx, until they ship inbox.

[davidsh]

If we decide to stop shipping the OOB in a future version it will be a breaking change

This issue is marked as 2.0. So, "future version" is now. We need to make a decision for 2.0.

[weshaggard]

Agreed the decision should be made in 2.0.

[karelz]

@weshaggard I lack the right technology terms in the space to describe what we decided. But what you said in https://github.com/dotnet/corefx/issues/16805#issuecomment-287155831 is exactly what I had in mind.
Let's sit down and think through all implications of such "radical" and technically breaking decision, but I think we should do it to lower the pain we inflict on our customers.

[gulbanana]

I would very happily give up those new apis to avoid the nightmare of binding redirects.

[ericstj]

I think to make an educated decision here we'd need to look at all consumers of those new APIs and get them to weigh in. My understanding is that the APIs were added specifically for our partners to create "useful" server libraries while not depending on platform specific technology (OpenSSL, WinHTTP). ASP.NET and Azure come to mind, but perhaps @terrajobst has a comprehensive source for that data.

Just getting rid of this source of bindingRedirects doesn't eliminate them. All of our NuGet packages that support desktop will version with every release so that they can be installed into the GAC without breaking existing apps. Non-CoreFx packages do the same. BindingRedirects are a reality of NuGet+Desktop.

[karelz]

@ericstj assemblies replacing Desktop binaries are in much worse position - we do not run Desktop tests (incl. for components upstream) with them overwritten.
However, for CoreFX packages sitting on top of Desktop, we do test the latest versions in CoreFX. There are no upstream dependencies in Framework itself.

[karelz]

BTW: Is GACing our packages (which sit on top of Desktop, but are not its replacements) a common scenario? Do we recommend it? Should we discourage it? (it won't fly for plugin systems like VS I guess)

[ericstj]

replacing Desktop binaries are in much worse position

I don't disagree with you, but you said bindingRedirects, not "replacing desktop components". From the bindingRedirects argument this is no different from anything else.

I agree replacing desktop components has more cost. Folks can decide to do it but they need to make sure all scenarios are tested. Once they do that it can actually provide some great wins for our customers. Look at MSBuild for example.

Is GACing our packages (which sit on top of Desktop, but are not its replacements) a common scenario?

Not common, but supported. We've definitely had folks do this in the past and I see some instances of it on my machines.

Do we recommend it?

No but we don't prevent it.

Should we discourage it?

Sometimes it is a requirement for a component based on how it is activated. It also happens to be how we service (in conjunction with publisher policy). So we need assembly versions to uniquely target components for servicing.

(it won't fly for plugin systems like VS I guess)

Yes, in fact, it will work for plugin systems. It won't work for plugins if things aren't versioned correctly.

[karelz]

@davidfowl can you please loop in right ASP.NET folks? -- @ericstj mentioned to me that you guys depend on the 8 new APIs we added and which we plan to pull in near future from the OOB package for Desktop. I would like to understand if the above plan makes sense end-to-end also for ASP.NET.

• What will be the impact on you?
• Will you be able to depend on older package for some time?
• Will you need to cross-compile for Desktop and use the "old" workaround APIs there? (details in Inheritance security rules violated by type: 'System.Net.Http.WebRequestHandler'. Derived types must either match the security accessibility of the base type or be less accessible. #18280 to post at the end)

[ericstj]

I clarified the title since we can't actually ship a facade for this since the contract name == inbox implementation name.

[karelz]

We have finalized the plan with @weshaggard: No work is needed in 2.0 branch.

Important notes - cc @davidsh @CIPop (@weshaggard please double-check my claims)

• System.Net.Http NuGet package does NOT exist in 2.0 branch anymore (recent work by @ericstj)
• System.Net.Http.dll assembly (for Desktop) DOES exist and its assembly version is bumped to 4.2.0.0
  • History of assembly versions of the package:
    • 4.0.0.0 (NuGet: N/A) - ships inbox in Desktop / .NET Framework (up to 4.7)
    • 4.1.0.0 (NuGet: 4.0.0) - shipped with .NET Core 1.0 RTW
    • 4.1.1.0 (NuGet: 4.1.0) - shipped with .NET Core 1.1 RTW
    • 4.1.1.1 (NuGet: 4.3.0) - shipped from .NET Core 1.1 servicing branch - patch for issue Inheritance security rules violated by type: 'System.Net.Http.WebRequestHandler'. Derived types must either match the security accessibility of the base type or be less accessible. #18280
    • 4.2.0.0 (NuGet: read for details) - will ship with .NET Core 2.0 RTW (not as separate NuGet package, but as part of NETStandard.Library.NETFramework NuGet package = Desktop support package)

If we need to ship patches to the OOB NuGet package, we will ship them from 1.1 servicing branch (or start building the package again from 2.x branches when needed).

The reason for NOT nuking also the assembly and redirecting it to Desktop:

• Whoever references the NuGet package today, references 4.1.0.0+ assembly version.
• To reference 4.0.0.0 assembly version, the person would have to downgrade bindingRedirect.
• It would be breaking more scenarios to avoid further potential/theoretical problems -- not the right tradeoff

If we see more problems with the package in future, we can always nuke the assembly later as I originally planned in this issue. It is just not worth it to do it proactively.