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

[Core] Don't throw from Response.Content if memory stream is private #21352

Merged
merged 1 commit into from
May 26, 2021

Conversation

ellismg
Copy link
Member

@ellismg ellismg commented May 25, 2021

Relax our constraint that the underlying buffer of the MemoryStream
backing a buffered response be publicly visible. There are cases today
where RequestBodyPolicy will not allocate a new MemoryStream. One
example is if the response stream is already seekable (as is the case
in our PlaybackTransport) nd we've observed that there are likely
cases where HttpClient itself may use a MemoryStream as the response
stream without allowing the underling buffer to be exposed.

In cases where we can not extract the underlying body, just make a
copy of it. Since we know the underlying stream is a memory stream, we
don't need to worry about hidden IO.

Fixes #21048

@ellismg ellismg requested review from pakrym and JoshLove-msft May 25, 2021 22:34
@ellismg ellismg requested a review from KrzysztofCwalina as a code owner May 25, 2021 22:34
@ghost ghost added the Azure.Core label May 25, 2021
@ellismg
Copy link
Member Author

ellismg commented May 25, 2021

I was never able to reproduce this issue when running against a live service, but we were able to hit it in playback. If we are worried about the case where the response content is a seekable stream, but not an exposable memory stream, (and so the early out we were doing was saving us a copy compared to what we would do with this change) we could try to do something more targeted in Response.Content where if the response is a MemoryStream but the buffer can't be expose we just use CopyTo to copy it to a new memory buffer, exploiting the fact that CopyTo on a memory stream won't do I/O.

But fixing things in the policy such that when buffering is requested, you always end up with an exposable memory stream as your stream felt like the right place to do this.

//
// We require that the underlying buffer of the memory stream be publicly visible because Azure.Response.Content uses
// TryGetBuffer to fetch the underlying data (to wrap it in a BinaryData) without copying.
if (!IsMemoryStreamWithPubliclyVisibleBuffer(responseContentStream))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the fix should be here or in the .Content implementation itself. Both would result in a copy but if we do it in the property it's pay-for-play.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about that as well - the one concern I had was if we would replace the ContentStream on the Response as part of doing this (so future calls to .Content would not need to copy) and if doing so would be weird because of things like:

Stream s1 = response.ContentStream;
Console.WriteLine(response.Content);
Stream s2 = response.ContentStream;

Object.ReferenceEquals(s1, s2) // could return false?!

Do you think this would be surprising in practice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not exactly what I've meant.

I was thinking of replacing

throw new InvalidOperationException($"The {nameof(ContentStream)}'s internal buffer cannot be accessed.");

with

return memoryContent.ToArray();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me, I suppose if we find that folks are calling .Content a bunch (and that's commonly leading to a bunch of copies) we can either figure a way to cache the value in Response (and eat the size increase) or explore something like swapping out the stream with a memory stream that exposes it's underlying buffer.

@ellismg ellismg force-pushed the ellismg/fix-21048 branch from 14b74f9 to aa83aca Compare May 26, 2021 00:37
@ellismg
Copy link
Member Author

ellismg commented May 26, 2021

@pakrym please take another look.

{
var response = new MockResponse(200);
response.ContentStream = new MemoryStream(new byte[100], 0, 100, writable: false, publiclyVisible: false);

Assert.Throws<InvalidOperationException>(() => { BinaryData d = response.Content; });
Assert.DoesNotThrow(() => { BinaryData d = response.Content; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe assert that the arrays have the same sequence?

@JoshLove-msft
Copy link
Member

nit: The PR description is no longer up to date.

@pakrym
Copy link
Contributor

pakrym commented May 26, 2021

Can you please add a changelog entry?

Relax our constraint that the underlying buffer of the MemoryStream
backing a buffered response be publicly visible. There are cases today
where `RequestBodyPolicy` will not allocate a new MemoryStream. One
example is if the response stream is already seekable (as is the case
in our PlaybackTransport) nd we've observed that there are likely
cases where `HttpClient` itself may use a MemoryStream as the response
stream without allowing the underling buffer to be exposed.

In cases where we can not extract the underlying body, just make a
copy of it. Since we know the underlying stream is a memory stream, we
don't need to worry about hidden IO.

Fixes Azure#21048
@ellismg ellismg force-pushed the ellismg/fix-21048 branch from aa83aca to 2520f57 Compare May 26, 2021 18:13
@ellismg ellismg changed the title [Core] Improve ContentStream buffering logic [Core] Don't throw from Response.Content if memory stream is private May 26, 2021
@ellismg ellismg enabled auto-merge (squash) May 26, 2021 18:14
@ellismg ellismg merged commit c8dda69 into Azure:master May 26, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this pull request Feb 14, 2023
Release apimangement 2022-08 01 (Azure#22313)

* Release api management 2022 08 01 (Azure#21169)

* add base for 2022-08-01

* updates readme.md

* update api-version and examples

* resolve Go SDK breaking change

* update examples to fix model definition

Co-authored-by: Chenjie Shi <tadelesh.shi@live.cn>

* Adding Resolver Entities to documentation (Azure#21352)

* merging in resolver entities

* adding resolver updates

* fixed policy examples that were missing a policyId param

* fixed typo that added a nested properties prop

* fixed description for API Resolvers List, filter query

* fixed error in definitions for resolvers

* fixed example that had an incorrect response definition

* added missing json file references

* fix for linting errors

* ref fixes and undoing bad merge overwrites

* fix typo

* wiki for apis and products  (Azure#21595)

* wiki for apis and products

* prettier; fixed spellchecks

* Fixed spelling

* Updated path

Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com>

* code review changes; gihub checks fixes

* fixed conact name and paths

* added properties back

* added contract properties objects

* fixed lint

* changed example to match the definition

* prettier

* code review changes

* added paths and examples for wiki collections

* Added x-ms-pageable

* removed count

Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com>

* Add ConfirmConsentCode to APIM RP (Azure#22418)

* Update apimauthorizationproviders.json

Add ConfirmConsentCode endpoint

* Update definitions.json

* Create ApiManagementPostAuthorizationConfirmConsentCodeRequest.json

* Update definitions.json

Remove count property

* Update apimauthorizationproviders.json

* [2022-08-01] Fix Linter Errors (Azure#22537)

* linter errors

* prettier

* v2

* fix error response duplicate schema

* type object

* prettier

* sync data from current ga version

* remove duplicate parameter

* type object

---------

Co-authored-by: Chenjie Shi <tadelesh.shi@live.cn>
Co-authored-by: hoonality <92482069+hoonality@users.noreply.github.com>
Co-authored-by: malincrist <92857141+malincrist@users.noreply.github.com>
Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com>
Co-authored-by: Logan Zipkes <44794089+LFZ96@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Request.Content throws InvalidOperationException when we expect it should not
3 participants