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

Small bug fixes for core. #28047

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

pallavit
Copy link
Contributor

@pallavit pallavit commented Apr 5, 2022

Description

  1. BearerTokenAuthenticationPolicy may cause a memory leak in case it is dealing with a request that returns a stream response. Adding the checks to make it resilient to it.
  2. MockHttpResponse does not return the original headers but creates a new list. This can prevent testing some scenarios where we update the original header values.

@azure-sdk
Copy link
Collaborator

API change check for com.azure:azure-core

API changes are not detected in this pull request for com.azure:azure-core

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jun 10, 2022
@ghost
Copy link

ghost commented Jun 10, 2022

Hi @pallavit. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@vcolin7
Copy link
Member

vcolin7 commented Jun 10, 2022

@alzimmermsft want to merge this PR?

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Jun 10, 2022
@alzimmermsft alzimmermsft merged commit d4f9765 into Azure:main Jun 13, 2022
Comment on lines +81 to +89
// Both Netty and OkHttp expect the requestBody to be closed after the response has been read.
// Failure to do so results in memory leak.
// In case of StreamResponse (or other scenarios where we do not eagerly read the response)
// the response body may not be consumed.
// This can cause potential leaks in the scenarios like above, where the policy
// may intercept the response and it may never be read.
// Forcing the read here - so that the memory can be released.
return httpResponse.getBody().ignoreElements()
.then(nextPolicy.process());
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw we should start using HttpResponse.close for this.

khmic5 pushed a commit to daniel-rocha/azure-sdk-for-java that referenced this pull request Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants