I am using HttpClient.SendAsync() to send restful service requests in .NET Framework (C#).

In some instances, I want to log the requests and the responses, for example when the response code is a particular HTTP status code, I'll want to log both the request and response.

The request (of course) is of type HttpRequestMessage and the response is of type HttpResponseMessage.

Here is the code I am working with right now:

protected async Task<HttpResponseMessage> SendAsyncInternal(object logContext, HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
{
    HttpResponseMessage response = null;
    Exception exception = null;
    try
    {
        response = await InstanceClient.SendAsync(request ?? throw new ArgumentNullException(nameof(request)), completionOption, cancellationToken);
    }
    catch (Exception e)
    {
        exception = e;
        throw;
    }
    finally
    {
        await LogDelegateInvoker(logContext, request, response, exception, cancellationToken);
    }
        
    return response;
}

The problem is that sometimes by the time my LogDelegateInvoker is invoked, the HttpClient.SendAsync() code has already disposed of the request or of the request content such that if my request tries to inspect (and log) that content, it can't because the content is already disposed.

I would love a way to tell the SendAsync() method NOT to dispose of the request and I will take responsibility for disposing of it myself.

Is there any way to do this, or another option?

CodePudding user response：

If you are using a .NET version above .NET Core 3.0, you should be able to avoid seeing a disposed request. (apparently there is a bug in older versions that HttpClient auto disposes the request.) with some easy changes. With the current code I see two issues that can be problematic:

1. LogDelegateInvoker can be called on a null request, since you are throwing a ArgumentNullException if request is null and catching it right away.
2. You are rethrowing the exception in your catch block which means that LogDelegateInvoker gets executed after the caller catch blocks are executed .(look at this for more info)) This will enable the calling code to dispose the request before your finally block gets called. It can also lead to the finally block never getting executed depending on the calling code.

In order to avoid both of these scenarios you can:

protected async Task<HttpResponseMessage> SendAsyncInternal(object logContext, HttpRequestMessage request,
    HttpCompletionOption completionOption, CancellationToken cancellationToken)
{
    var httpClient = new HttpClient();
    if (request == null)
    {
        throw new ArgumentNullException(nameof(request));
    }

    HttpResponseMessage response = null;
    Exception exception = null;
    try
    {
        response = await httpClient.SendAsync(request, completionOption, cancellationToken);
    }
    catch (Exception e)
    {
        exception = e;
    }
    finally
    {
        await LogDelegateInvoker(logContext, request, response, exception, cancellationToken);
    }

    if (exception != null)
    {
        throw exception;
    }

    return response;
}

which should prevent the LogDelegateInvoker from being called on a dispose request.

CodePudding user response：

I have found an answer that is acceptable for me, and may be useful for others.

I've replaced the code in my question with this:

protected async Task<HttpResponseMessage> SendAsyncInternal(object logContext, HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
{
    LoggingForHttpHandler.AssociateInvokerWithRequest(request ?? throw new ArgumentNullException(nameof(request)), 
        async (req, res, ex, ct) => await LogDelegateInvoker(logContext, req, res, ex, ct));
    return await HttpClient.SendAsync(request, completionOption, cancellationToken);
}

Then in that same class, I have and additional private child class and some static handling:

...

private static HttpClient HttpClient => _httpClient ?? (_httpClient = new HttpClient(new LoggingForHttpHandler(new HttpClientHandler())));
private static HttpClient _httpClient;

private class LoggingForHttpHandler : DelegatingHandler
{
    public LoggingForHttpHandler(HttpMessageHandler innerHandler) : base(innerHandler)
    {
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        HttpResponseMessage response = null;
        Func<HttpRequestMessage, HttpResponseMessage, Exception, CancellationToken, Task> logDelegateInvoker = null;

        try
        {
            if (request.Headers.TryGetValues(SpecialHeaderName, out IEnumerable<string> specialHeaderValues))
            {
                request.Headers.Remove(SpecialHeaderName);
                if (ulong.TryParse(specialHeaderValues.FirstOrDefault(), out ulong logDelegateInvokerKey))
                {
                    if (!LogDelegateInvokers.TryRemove(logDelegateInvokerKey, out logDelegateInvoker))
                    {
                        logDelegateInvoker = null;
                    }
                }
            }

            response = await base.SendAsync(request, cancellationToken);
        }
        catch(Exception ex)
        {
            if (logDelegateInvoker != null)
            {
                await logDelegateInvoker(request, response, ex, cancellationToken);
            }
            throw;
        }

        if (logDelegateInvoker != null)
        {
            await logDelegateInvoker(request, response, null, cancellationToken);
        }

        return response;
    }

    public static void AssociateInvokerWithRequest (HttpRequestMessage request, Func<HttpRequestMessage, HttpResponseMessage, Exception, CancellationToken, Task> logDelegateInvoker)
    {
        if (logDelegateInvoker != null && request != null)
        {
            ulong logDelegateInvokerKey = (ulong)(Interlocked.Increment(ref _incrementer) - long.MinValue);
            if (LogDelegateInvokers.TryAdd(logDelegateInvokerKey, logDelegateInvoker))
            {
                request.Headers.Add(SpecialHeaderName, logDelegateInvokerKey.ToString());
            }

        }
    }

    private const string SpecialHeaderName = "__LogDelegateIndex";

    private static long _incrementer = long.MinValue;

    private static readonly ConcurrentDictionary<ulong, Func<HttpRequestMessage, HttpResponseMessage, Exception, CancellationToken, Task>> LogDelegateInvokers = 
        new ConcurrentDictionary<ulong, Func<HttpRequestMessage, HttpResponseMessage, Exception, CancellationToken, Task>>();
}

...

The key point here is to create the client with a custom delegating handler and to set it up so that delegating handler will actually invoke the delegate for the logging before the request is disposed. What is done to support this is to have a static threadsafe dictionary of invocations indexed by a 64 bit key, which is rendered as a string as "special" header on the request before it is submitted, and then stripped from the header before it is sent. Note that header access on requests of type HttpRequestMessage is synchronous.

I acknowledge that this doesn't have perfect code smell, but it is efficient and fast.

I would still be thrilled to get a better solution!