22

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.

Camilo Terevinto
  • 31,141
  • 6
  • 88
  • 120
PaulMiami
  • 665
  • 1
  • 6
  • 17
  • it'll will dispose itself once its out of scope and not in (async) use. – Paul Swetz Jun 28 '16 at 18:03
  • Of course the CG will eventually finalize them, but is this the best practice to do so under this circumstance and why? – PaulMiami Jun 28 '16 at 18:29
  • If dispose is called before the async method finishes you will throw exceptions. – Paul Swetz Jun 28 '16 at 18:36
  • Possible duplicate of [How to dispose asynchronously?](http://stackoverflow.com/questions/400130/how-to-dispose-asynchronously) – Paul Swetz Jun 28 '16 at 18:36
  • This discussion is relevant. http://stackoverflow.com/questions/400130/how-to-dispose-asynchronously – Paul Swetz Jun 28 '16 at 18:37
  • True, but not the same, in this case the above code is being awaited – PaulMiami Jun 28 '16 at 18:52
  • If you really want to get to the nitty gritty wrap the HttpRequestMessage in a using block and I'm guessing you can watch it explode on you depending on how long the response takes to come in. – Paul Swetz Jun 28 '16 at 18:55
  • 1
    Post your question in the .NET CoreFx repo: https://github.com/dotnet/corefx/issues ...AFAIK for example for response, the `ReadAsString` methods consume the response stream and dispose it for you and so you need not dispose the httpresponsemessage yourself... – Kiran Jun 28 '16 at 19:08
  • 1
    This discussion is relevant. [http://stackoverflow.com/a/16566605/6524718](http://stackoverflow.com/a/16566605/6524718) – PaulMiami Jun 28 '16 at 19:18
  • Thanks @KiranChalla, I have opened an issue on github [https://github.com/aspnet/Security/issues/886](https://github.com/aspnet/Security/issues/886) – PaulMiami Jun 28 '16 at 19:38
  • 1
    good question and research Paul Miami , I was just wondering about that as well. – RedRose Aug 31 '22 at 16:45

1 Answers1

20

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.

Community
  • 1
  • 1
PaulMiami
  • 665
  • 1
  • 6
  • 17
  • 1
    Just want to refer to your first quote, if .Net Core is used. Visiting https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/HttpClient.cs#L549 (for .Net Core) shows that it has changed (on commit 6d06c39, April 28 2017) so now the request content won't be disposed: // This method used to also dispose of the request content, e.g.: // request.Content?.Dispose(); // This has multiple problems: .... – Hummus Oct 25 '18 at 14:55
  • I've seen the source code and it also seems to indicate that httpResponseMessage.Dispose just disposes the Content, but when I dispose only the response and not the Content, I see errors that I don't see when I dispose first the Content and then also the response. It makes no sense, so I'm going to try debugging the source code. As far as the request codes, the question is, when should it be disposed. Once an HttpResponseMessage is returned? – Triynko Dec 20 '20 at 05:05
  • Is there a reason why `HttpResponseMessage` does not implement `IAsyncDisposable` ? – kofifus Aug 21 '21 at 01:57