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

AccessTokenHandler does not support sync methods #28

Closed
codingbott opened this issue Nov 7, 2024 · 2 comments · Fixed by #90
Closed

AccessTokenHandler does not support sync methods #28

codingbott opened this issue Nov 7, 2024 · 2 comments · Fixed by #90
Labels
area/access-token-management Issues related to Access Token Management impact/non-breaking The fix or change will not be a breaking one priority/2 High

Comments

@codingbott
Copy link

Which version of Duende.AccessTokenManagement are you using?
3.0.0

Which version of .NET are you using?
6

Describe the bug

I'm try to use the OpenTelemetryClient from dotnet together with the AccessTokenManagement.

Setup like this:

builder.Services.AddClientCredentialsTokenManagement().AddClient("Token.Client.OtelAuditLogger", options =>
                {
                   // options setup here with crediential etc.
                });

            builder.Services
                .AddClientCredentialsHttpClient("Http.Client.OtelAuditLogger", "Token.Client.OtelAuditLogger");

Later on in my implementation I pass the IHttpClientFactory in my class:

    public AuditLogger(IHttpClientFactory httpClientFactory)
    {
        string plantShort = "tc5ot";
        string teamName = "BackboneInfra";
        string applicationName = "OtelDemoApp";

        var loggerFactory = LoggerFactory.Create(builder =>
        {
            builder.SetMinimumLevel(LogLevel.Trace);
            builder.AddOpenTelemetry(logging =>
            {
                logging.AddOtlpExporter(otlpOptions =>
                {
                    otlpOptions.Endpoint = new Uri(OtlpHttp);
                    otlpOptions.Protocol = OtlpExportProtocol.HttpProtobuf;

                    otlpOptions.HttpClientFactory = () =>
                    {
                        var httpClient = httpClientFactory.CreateClient("Http.Client.OtelAuditLogger");
                        httpClient.BaseAddress = new Uri(OtlpHttp);

                        // this lines test if the AccessTokenManagement is bound
                        // using SEND does NOT request a token and SEND is used by OTEL log exporter 
                        var failingGettingToken = httpClient.Send(new HttpRequestMessage());
                        // compared with SendAsync: This does request a token
                        var successGettingToken = httpClient.SendAsync(new HttpRequestMessage());

                        return httpClient;
                    };
                });
            });
        });

        var _logger = loggerFactory.CreateLogger<AuditLogger>();
        _logger.Log(LogLevel.Critical, "Hello World");
    }

To Reproduce

See the source above. Where the httpClient is created I'm doing a send which does not request tokens. I saw that in the logs.
When I call SendAsync, then the lib is requesting a token.

Expected behavior

Also Send method will request a token.

Additional context

src/Duende.AccessTokenManagement/AccessTokenHandler.cs

Does not override the Send method, but SendAync only.

@AndersAbel
Copy link
Member

It looks like Send was added in .NET 5, while SendAsync has been around since .NET Core 1.0 (and .NET Framework 4.5), so I guess the reason is that when the code was initially written, there was no Send method.

I'll transfer this issue to our FOSS repository (the home for Duende.AccessTokenManagement) for handling.

@AndersAbel AndersAbel transferred this issue from DuendeSoftware/Support Nov 11, 2024
@AndersAbel AndersAbel added area/access-token-management Issues related to Access Token Management state/needs-triage Needs triaging by the maintainers labels Nov 11, 2024
@josephdecock josephdecock added priority/2 High impact/non-breaking The fix or change will not be a breaking one and removed state/needs-triage Needs triaging by the maintainers labels Nov 13, 2024
@Erwinvandervalk
Copy link
Contributor

Erwinvandervalk commented Feb 5, 2025

Thank you for bringing this to our attention.

I understand that the open telemetry library invokes the synchronous send operation, which is currently not supported. .

Unfortunately, the only way we could implement a synchronous Send() is by doing .GetAwaiter().GetResult(), which is absolutely not a recommended practice and not something we're considering implementing. However, we should fail fast when the synchronous Send() method is invoked. So this is a change that we'll introduce in the near future.

In the open telemetry library there's a PR and a big discussion associated with it to make this functionality async (open-telemetry/opentelemetry-dotnet#5838). Now this PR isn't merged yet and no ETA on this,

Should you accept the issues and risks associated with calling .GetAwaiter().GetResult(), you can always create your own subclass of the Handler that doesn't throw like this:

public class SynchronousOpenIdConnectClientAccessTokenHandler : OpenIdConnectClientAccessTokenHandler
{
    public SynchronousOpenIdConnectClientAccessTokenHandler(IDPoPProofService dPoPProofService, IDPoPNonceStore dPoPNonceStore, IHttpContextAccessor httpContextAccessor, ILogger<OpenIdConnectClientAccessTokenHandler> logger, UserTokenRequestParameters? parameters = null) : base(dPoPProofService, dPoPNonceStore, httpContextAccessor, logger, parameters)
    {
    }

    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        // Please note that this code (depending on where it's invoked) is subject to deadlocks and thread pool starvation
        return base.SendAsync(request, cancellationToken).GetAwaiter().GetResult();
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/access-token-management Issues related to Access Token Management impact/non-breaking The fix or change will not be a breaking one priority/2 High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants