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

Change EnsureSuccessStatusCode to not dispose response content #29795

Merged
merged 2 commits into from May 19, 2018

Conversation

Projects
None yet
5 participants
@davidsh
Copy link
Member

commented May 19, 2018

Historically, this method has always disposed the response content if it was
throwing an exception. The reason documented in the comment really doesn't
make much sense, especially to just dispose the content but not the actual
response object. The developer pattern we advise when getting an HttpResponseMessage
is to wrap it in a 'using' statement or use an explicit .Dispose() call. And
disposing the HttpResponseMessage would always dispose the inner .Content
anyways.

The use case of #26684 is that the response content should be available regardless
if this method throws an exception or not. Developers normally don't expect an object
to get disposed (partially in this case) as a side-affect of calling a method on the
object. So, we're changing the behavior to be more consistent generally with other
.NET objects.

Fixes #26684

Change EnsureSuccessStatusCode to not dispose response content
Historically, this method has always disposed the response content if it was
throwing an exception. The reason documented in the comment really doesn't
make much sense, especially to just dispose the content but not the actual
response object. The developer pattern we advise when getting an HttpResponseMessage
is to wrap it in a 'using' statement or use an explicit .Dispose() call. And
disposing the HttpResponseMessage would always dispose the inner .Content
anyways.

The use case of #26684 is that the response content should be available regardless
if this method throws an exception or not. Developers normally don't expect an object
to get disposed (partially in this case) as a side-affect of calling a method on the
object. So, we're changing the behavior to be more consistent generally with other
.NET objects.

Fixes #26684

@davidsh davidsh added this to the 2.2.0 milestone May 19, 2018

@davidsh davidsh self-assigned this May 19, 2018

@davidsh davidsh requested review from stephentoub and dotnet/ncl May 19, 2018

@@ -120,13 +120,13 @@ public void EnsureSuccessStatusCode_VariousStatusCodes_ThrowIfNot2xx()
}

[Fact]
public void EnsureSuccessStatusCode_AddContentToMessage_ContentIsDisposed()
public void EnsureSuccessStatusCode_AddContentToMessage_ContentIsNotDisposed()

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 19, 2018

Member

Does this need to be skipped now on netfx, since it doesn't have the change?

This comment has been minimized.

Copy link
@davidsh

davidsh May 19, 2018

Author Member

Yes. Forgot about that. I'll update the test.

@stephentoub stephentoub merged commit b0e049c into dotnet:master May 19, 2018

13 of 14 checks passed

Windows x64 Debug Build Build finished.
Details
CROSS Check Build finished.
Details
Linux arm Release Build Build finished.
Details
Linux arm64 Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
WIP ready for review
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details

@davidsh davidsh deleted the davidsh:EnsureSuccessStatusCode_nodispose branch May 19, 2018

stephentoub added a commit that referenced this pull request May 21, 2018

hughbe added a commit to hughbe/corefx that referenced this pull request May 29, 2018

Change EnsureSuccessStatusCode to not dispose response content (dotne…
…t#29795)

* Change EnsureSuccessStatusCode to not dispose response content

Historically, this method has always disposed the response content if it was
throwing an exception. The reason documented in the comment really doesn't
make much sense, especially to just dispose the content but not the actual
response object. The developer pattern we advise when getting an HttpResponseMessage
is to wrap it in a 'using' statement or use an explicit .Dispose() call. And
disposing the HttpResponseMessage would always dispose the inner .Content
anyways.

The use case of dotnet#26684 is that the response content should be available regardless
if this method throws an exception or not. Developers normally don't expect an object
to get disposed (partially in this case) as a side-affect of calling a method on the
object. So, we're changing the behavior to be more consistent generally with other
.NET objects.

Fixes dotnet#26684

* Address PR feedback

satano added a commit to satano/corefx that referenced this pull request May 30, 2018

Change EnsureSuccessStatusCode to not dispose response content (dotne…
…t#29795)

* Change EnsureSuccessStatusCode to not dispose response content

Historically, this method has always disposed the response content if it was
throwing an exception. The reason documented in the comment really doesn't
make much sense, especially to just dispose the content but not the actual
response object. The developer pattern we advise when getting an HttpResponseMessage
is to wrap it in a 'using' statement or use an explicit .Dispose() call. And
disposing the HttpResponseMessage would always dispose the inner .Content
anyways.

The use case of dotnet#26684 is that the response content should be available regardless
if this method throws an exception or not. Developers normally don't expect an object
to get disposed (partially in this case) as a side-affect of calling a method on the
object. So, we're changing the behavior to be more consistent generally with other
.NET objects.

Fixes dotnet#26684

* Address PR feedback
@cremor

This comment has been minimized.

Copy link

commented Jun 16, 2018

@davidsh @stephentoub The documentation should be updated to reflect this change.

@karelz

This comment has been minimized.

Copy link
Member

commented Jun 16, 2018

@cremor what would you document and where? Can you submit PR in docs repo? (hit edit button on the MSDN docs page)

@davidsh

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2018

Remarks
The EnsureSuccessStatusCode method throws an exception if the HTTP response was unsuccessful. If the Content is not null, this method will also call Dispose to free managed and unmanaged resources.

This PR was merged after the .NET Core 2.1 branch was finalized. So, the documentation is still correct.

However, starting after .NET Core 2.1, this sentence is no longer true. It should be removed at that time from the documentation:
"If the Content is not null, this method will also call Dispose to free managed and unmanaged resources."

@cremor

This comment has been minimized.

Copy link

commented Jun 16, 2018

@karelz Sorry for being so unspecific. @davidsh perfectly described what I meant.

@karlzf

This comment has been minimized.

Copy link

commented Feb 16, 2019

However, starting after .NET Core 2.1, this sentence is no longer true. It should be removed at that time from the documentation:
"If the Content is not null, this method will also call Dispose to free managed and unmanaged resources."

I did not see the changes applied to 2.2 release: https://github.com/dotnet/corefx/blob/release/2.2/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs. Can anyone confirm if the comment by @davidsh is still valid?

@stephentoub

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

It's a bit confusing, but coreclr and corefx have very few changes between 2.1 and 2.2, only critical fixes. Most changes that went into corefx after 2.1, including this one, will be in 3.0, which is the next major rev of the runtime and libraries. 2.2 was primarily about improvements to ASP.NET.

@karlzf

This comment has been minimized.

Copy link

commented Feb 24, 2019

Thanks for clarifications @stephentoub

davidsh added a commit to davidsh/dotnet-api-docs that referenced this pull request Feb 28, 2019

Update HttpResponseMessage.EnsureSuccessStatusCode method
Starting with .NET Core 3.0, the EnsureSuccessStateCode method no longer
disposes the HttpResponseMessage.Content object.

Reference: dotnet/corefx#29795 (comment)
@davidsh

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

I created a documentation PR dotnet/dotnet-api-docs#1971 to update the docs for .NET Core 3.0 to reflect this new behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.