Remove HttpClient as a registered service

[brettsam]

Discussion for the breaking change proposal: #1911

[fabiocav]

About this blurb:

If we remove all usage of HttpClient internally, we could maintain a DI factory so that, if called, it logs an exception that causes a host failure. Without an HttpClient registered, customers will get a null ref in their constructors that have an HttpClient parameter.

We should add that to 3.x today and log usage (similar do your proposed process for the 4.x exception) and log that metric.

Also adding @ahmelsayed as I believe he had some of the dependencies on this.

[brettsam]

Yes, that should work. I think we could fix this in v3 without breaking anyone by:

1. removing the WebHost HttpClient registration and fixing everywhere that needs it
2. registering a JobHost-level HttpClient (with logging upon creation)
3. in v4 -- change the JobHost-level client creation to be an error (this would be the break).

As for your second note, I do see HttpClient being used in a handful of constructors. I'll need to evaluate all the usages and likely swap them to using IHttpClientFactory.

[brettsam]

After discussion, the decision was:

• add telemetry to log this behavior in v3 so it's able to be detected
• fix fully in v4.
• outstanding discussions on exactly what to do in v4:
  • do we explicitly throw with a link to the breaking change?
  • if we do not register any HttpClient, customer may get confusing error.

[cgillum]

Regarding this:

The HttpClient class was registered with DI early in Functions v2 and we didn't realize the mistake until we enabled Dependency Injection for .NET in-proc applications.

Why was this a mistake? The motivation for this breaking change isn't clear to me.

[brettsam]

Registering a singleton HttpClient is the wrong pattern in .NET Core (as of 2.1). Instead, you should use an IHttpClientFactory which has a handful of benefits. Best explained in the docs here.

We also hit some DI-related issues with the way it is registered at the app level. This is the issue I recall, but I think there were others.

Side note... our docs are still incorrect even though I updated them here. I'm not sure what happened; will follow up.

[cgillum]

Oh! Sorry, I totally misread the issue - I thought you were removing IHttpClientFactory. This makes perfect sense - thanks!

[fabiocav]

@eluchsinger we'll follow up on the issue in the worker as part of triage and provide an update there. I'll mark this as hidden for now as it isn't related to the breaking change proposal. Thanks!