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.

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

Microsoft.Extensions.Http.Polly 5.0.0 broke AddHttpClient - configureClient not called #45035

Closed
cmeeren opened this issue Nov 19, 2020 · 13 comments

Comments

@cmeeren
Copy link

cmeeren commented Nov 19, 2020

After updating from 3.1.10 to 5.0.0, the configureClient parameter of AddHttpClient is no longer called. In particular, it is the overload that accepts string and Action<HttpClient> I have tried. I have not tested others. Reverting to 3.1.10 fixes the issue.

The relevant code (F#):

type Startup(config: IConfiguration) =

  member __.ConfigureServices(services: IServiceCollection) : unit =
    services
      .AddHttpClient(HttpClientNames.chkbox, fun client -> ... )

Hopefully this is trivial to reproduce. Let me know if not.

Additional context

Output of dotnet --info:

.NET SDK (reflecting any global.json):
 Version:   5.0.100
 Commit:    5044b93829

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19041
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100\

Host (useful for support):
  Version: 5.0.0
  Commit:  cf258a14b7

.NET SDKs installed:
  3.1.100 [C:\Program Files\dotnet\sdk]
  3.1.404 [C:\Program Files\dotnet\sdk]
  5.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
@cmeeren cmeeren changed the title 5.0.0 broke AddHttpClient - configureClient not called Microsoft.Extensions.Http.Polly 5.0.0 broke AddHttpClient - configureClient not called Nov 19, 2020
@BrennanConroy
Copy link
Member

Polly seems like a re-herring here.

It's likely updating from 3.1.x of HttpClientFactory to 5.0.x is the real change here.

@BrennanConroy BrennanConroy transferred this issue from dotnet/extensions Nov 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-HttpClientFactory untriaged New issue has not been triaged by the area owner labels Nov 20, 2020
@ghost
Copy link

ghost commented Nov 20, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

After updating from 3.1.10 to 5.0.0, the configureClient parameter of AddHttpClient is no longer called. In particular, it is the overload that accepts string and Action<HttpClient> I have tried. I have not tested others. Reverting to 3.1.10 fixes the issue.

The relevant code (F#):

type Startup(config: IConfiguration) =

  member __.ConfigureServices(services: IServiceCollection) : unit =
    services
      .AddHttpClient(HttpClientNames.chkbox, fun client -> ... )

Hopefully this is trivial to reproduce. Let me know if not.

Additional context

Output of dotnet --info:

.NET SDK (reflecting any global.json):
 Version:   5.0.100
 Commit:    5044b93829

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19041
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100\

Host (useful for support):
  Version: 5.0.0
  Commit:  cf258a14b7

.NET SDKs installed:
  3.1.100 [C:\Program Files\dotnet\sdk]
  3.1.404 [C:\Program Files\dotnet\sdk]
  5.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Author: cmeeren
Assignees: -
Labels:

area-Extensions-HttpClientFactory, untriaged

Milestone: -

@CarnaViire
Copy link
Member

@cmeeren Unfortunately I wasn't able to reproduce the issue. Could you please provide a minimal repro?

@cmeeren
Copy link
Author

cmeeren commented Nov 23, 2020

Here's a repro:

Test.zip

@BrennanConroy was right in that Polly was a red herring. If you target net5.0 (as the repro is), the bug is there. If you change the target to netcoreapp3.1, it works fine.

The relevant code:

open System
open System.Net.Http
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.DependencyInjection
open Microsoft.Extensions.Hosting


type Startup() =

  member _.ConfigureServices(services: IServiceCollection) : unit =
    services.AddHttpClient("test", fun client -> client.BaseAddress <- Uri("https://example.com"))
    |> ignore


  member _.Configure(app: IApplicationBuilder) : unit =
    let factory = app.ApplicationServices.GetService<IHttpClientFactory>()
    let client = factory.CreateClient("test")
    Console.WriteLine(
      "\nCLIENT BASE ADDRESS = "
      + if isNull client.BaseAddress then "NULL" else client.BaseAddress.ToString()
      + "\n"
    )


[<EntryPoint>]
let main args =
  Host
    .CreateDefaultBuilder(args)
    .ConfigureWebHostDefaults(fun builder ->
       builder.UseStartup<Startup>()|> ignore)
    .Build()
    .Run()
  0

Expected output (and actual output with netcoreapp3.1):

CLIENT BASE ADDRESS = https://example.com/

Actual output (with net5.0):

CLIENT BASE ADDRESS = NULL

@CarnaViire
Copy link
Member

Thank you @cmeeren! It turns out the issue happens only with F#.

The problem here is that the wrong overload of AddHttpClient is called for some reason. The one that you expected to be called (and the one that C# calls without problems) is

AddHttpClient(this IServiceCollection services, string name, Action<HttpClient> configureClient)

(link)

but the one that gets actually called from F# is

AddHttpClient<TClient,TImplementation>(this IServiceCollection services, string name, Func<HttpClient,TImplementation> factory)

(link)

where both TClient and TImplementation are the unit type (link). The result of the wrong method being called is that HttpClient is registered on the name, but the action is not treated as HttpClient configuration but as an instance factory for a typed client and therefore is not called when HttpClient is created.

The latter AddHttpClient method was added in 5.0, that's why it doesn't fail on 3.1.

It seems that F# treats Action<T> as Func<T,unit>, but it still should be able to tell between two overloads. We will investigate this further.

Meanwhile, you may use the following workaround

services.AddHttpClient("test")
        .ConfigureHttpClient(fun client -> client.BaseAddress <- Uri("https://example.com"))

Please let me know if the workaround worked for you.

@cmeeren
Copy link
Author

cmeeren commented Nov 24, 2020

Thank you for looking into this! I can confirm that the workaround works.

@CarnaViire
Copy link
Member

Hey @eiriktsarpalis my team suggested that you could take a look at this, because you know F# well 😃 Could you please help us to understand what is happening?

@halter73
Copy link
Member

halter73 commented Nov 25, 2020

Another workaround is to a named argument for the AddHttpClient configuration callback

  member _.ConfigureServices(services: IServiceCollection) : unit =
    services.AddHttpClient("test", configureClient = fun client -> client.BaseAddress <- Uri("https://example.com"))
    |> ignore

This works because the incorrect overload's callback is named factory not configureClient. It's too bad F# prefers the overload expecting Func<T, TResult> over the overload that takes Action<T> when the lambda signature is T -> unit. Maybe that can be fixed in the F# compiler.

@eiriktsarpalis
Copy link
Member

This is a known limitation of F# delegate interop. In order to support currying, F# native lambdas use their own dedicated type but the compiler is generally capable of implicitly converting lambda literals into delegate types, sometimes requiring a bit of help.

There are a few ways you can provide hints to the compiler so that the right overload is resolved (other than the one provided by @halter73):

In this case, constraining the type parameter arity should suffice:

  member _.ConfigureServices(services: IServiceCollection) : unit =
    services.AddHttpClient<_>("test", fun client -> client.BaseAddress <- Uri("https://example.com"))
    |> ignore

My personal preference however is to just explicitly define the delegate type, which should make your code more resilient against potential new overloads breaking your code in the future:

  member _.ConfigureServices(services: IServiceCollection) : unit =
    services.AddHttpClient("test", Action<HttpClient>(fun client -> client.BaseAddress <- Uri("https://example.com")))
    |> ignore

That being said, it feels to me that the compiler should have produced an error here. @cartermp thoughts?

@eiriktsarpalis
Copy link
Member

It's too bad F# prefers the overload expecting Func<T, TResult> over the overload that takes Action when the lambda signature is T -> unit. Maybe that can be fixed in the F# compiler.

I should note that unit is not a synonym for void. It is a singleton type and lambdas of type 'T -> unit are represented as FSharpFunc<'T, unit>, equivalent to Func<'T, unit>.

@cartermp
Copy link

Yep, I think @eiriktsarpalis's suggestion about being explicit w.r.t Action. vs. Func for the parameter in question is the best overall approach.

Note: I still consider myself a novice when it comes to the intricacies of overload resolution. As far as I can tell, F# will prefer the Func over the Action when both are present, which you can see in this sharplab. Ordering doesn't matter. If you remove the Func overload, the Action one is picked and it compiles just fine. Since this behavior holds for a simple F# snippet in purely F# code, I don't think there's much to do here. It's just the behavior of the compiler that likely goes back to at least F# 4.0 (I mention this only because this is the earliest point it was fully OSS; the pre-OSS history is something I just can't speak towards).

There's a general problem of F# type inference combined with overloads that is unavoidable - an added overload can be a breaking change for F# callers that rely on type inference - and this is what hit. This is an example of that. Using explicit types or named parameters can help.

@cartermp
Copy link

Perhaps the Action vs. Func stuff is worth documenting as guidance in the F# docs. It's common enough to encounter methods that accept on or the other and it's always kind of a pain to interop with them.

@CarnaViire
Copy link
Member

@eiriktsarpalis @cartermp thanks a lot for the explanations!

@cartermp can you please link the F# docs issue once you file?
Closing this issue, as it is general F# limitation, not much we can do about it here.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2020
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants