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

Ensure BufferedResponse type is not caching un-released buffer #7837

Merged
merged 4 commits into from
Jan 31, 2020

Conversation

anuchandy
Copy link
Member

No description provided.

@anuchandy
Copy link
Member Author

The issue:

The BufferedHttpResponse used to cache only references to the source response body buffers. The buffers will be released underneath as first subscription consumes them. For the future subscriptions these released buffers were severed, this result in decoder and other parts depending on BufferedHttpResponse to receive garbage data.

consumers of BufferedHttpResponse:

  1. test record interceptor
  2. body decoder
  3. http logging interceptor

fixes in pr:

  1. updates the BufferedHttpResponse to do deep copy caching of source response body buffers.
  2. removes BufferedHttpResponse use in "body decoder" (that saves extra allocations)
  3. updates StreamResponse::close() such that it won't attempt to dispose already consumed buffer source.

No update in "test record interceptor" and is fine to buffer as this is not used in production.

future

  1. we could improve "http logging interceptor" by not doing buffering. This is not included in this PR.

Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

Looks good to me. Not certain either if my suggestions will work but something to think about applying (and for future coding efforts to cut down on verbosity).

@anuchandy
Copy link
Member Author

@srnagar thanks for the review. @JonathanGiles, thanks - your suggestions work, validated locally so committing them.

anuchandy and others added 2 commits January 30, 2020 14:31
…n/http/BufferedHttpResponse.java

Co-Authored-By: Jonathan Giles <[email protected]>
…n/http/BufferedHttpResponse.java

Co-Authored-By: Jonathan Giles <[email protected]>
@anuchandy anuchandy merged commit 5591650 into Azure:master Jan 31, 2020
srnagar pushed a commit to srnagar/azure-sdk-for-java that referenced this pull request Feb 4, 2020
…#7837)

* Making BufferedResponse to do deep caching (it was doing shallow caching)

* Make BufferedResponse really buffer and updating decoder to not to use BufferedResponse

* Update sdk/core/azure-core/src/main/java/com/azure/core/implementation/http/BufferedHttpResponse.java

Co-Authored-By: Jonathan Giles <[email protected]>

* Update sdk/core/azure-core/src/main/java/com/azure/core/implementation/http/BufferedHttpResponse.java

Co-Authored-By: Jonathan Giles <[email protected]>

Co-authored-by: Jonathan Giles <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants