Not calling Dispose on HttpRequestMessage and HttpResponseMessage in asp.net core

Question

What is the best practice for calling Dispose (or not) on HttpRequestMessage and HttpResponseMessage with asp.net core?

Examples:

https://github.com/aspnet/Security/blob/1.0.0/src/Microsoft.AspNetCore.Authentication.Google/GoogleHandler.cs#L28-L34

protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens)
{
    // Get the Google user
    var request = new HttpRequestMessage(HttpMethod.Get, Options.UserInformationEndpoint);
    request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.AccessToken);

    var response = await Backchannel.SendAsync(request, Context.RequestAborted);
    response.EnsureSuccessStatusCode();

    var payload = JObject.Parse(await response.Content.ReadAsStringAsync());
    ...
 }

and https://github.com/aspnet/Security/blob/1.0.0/src/Microsoft.AspNetCore.Authentication.Facebook/FacebookHandler.cs#L37-L40
Both examples are not calling Dispose

Could this be an omission? or is there a valid reason behind it, maybe because the method is async? Of course the CG will eventually finalize them, but is this the best practice to do so under this circumstance and why? Notice that the above examples are part of ASP.NET Core Middleware components.

Answer 1

I opened an issue on the github repository where the code examples belong to.

https://github.com/aspnet/Security/issues/886

It's not important in these scenarios. Disposing a request or response only calls Dispose on their Content field. Of the various HttpContent implementations, only StreamContent needs to dispose anything. HttpClient's default SendAsync fully buffers the response content and disposes the stream, so there's nothing the caller needs to do.

But for the sake of not getting weird bugs down the line, we are better off disposing those object. MemoryStream is another class that is also often not dispose because of his current underlying implementation.

https://stackoverflow.com/a/234257/6524718

If you're absolutely sure that you never want to move from a MemoryStream to another kind of stream, it's not going to do you any harm to not call Dispose. However, it's generally good practice partly because if you ever do change to use a different Stream, you don't want to get bitten by a hard-to-find bug because you chose the easy way out early on. (On the other hand, there's the YAGNI argument...)

The other reason to do it anyway is that a new implementation may introduce resources which would be freed on Dispose.