Skip to content

Close response body in bearer policy #40052

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.

Sign up for GitHub

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

Merged
lmolkova merged 1 commit into from
May 6, 2024
Merged

Close response body in bearer policy #40052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 7 additions & 17 deletions .../azure-core/src/main/java/com/azure/core/http/policy/BearerTokenAuthenticationPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,11 @@ public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineN
return authorizeRequest(context).then(Mono.defer(next::process)).flatMap(httpResponse -> {
String authHeader = httpResponse.getHeaderValue(HttpHeaderName.WWW_AUTHENTICATE);
if (httpResponse.getStatusCode() == 401 && authHeader != null) {
return authorizeRequestOnChallenge(context, httpResponse).flatMap(retry -> {
if (retry) {
// 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
Author

@lmolkova lmolkova May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

closing is better than reading to the end in edge cases:

  • we don't need to waste time/resources on reading (unlikely) big response
  • if there is a transient issue during stream reading, we don't need to handle/retry it.

return authorizeRequestOnChallenge(context, httpResponse).flatMap(authorized -> {
if (authorized) {
// body needs to be closed or read to the end to release the connection
httpResponse.close();
return nextPolicy.process();
} else {
return Mono.just(httpResponse);
}
Expand All @@ -155,13 +150,8 @@ public HttpResponse processSync(HttpPipelineCallContext context, HttpPipelineNex
String authHeader = httpResponse.getHeaderValue(HttpHeaderName.WWW_AUTHENTICATE);
if (httpResponse.getStatusCode() == 401 && authHeader != null) {
if (authorizeRequestOnChallengeSync(context, httpResponse)) {
// 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.
// body needs to be closed or read to the end to release the connection
httpResponse.close();
return nextPolicy.processSync();
} else {
return httpResponse;
Expand Down
2 changes: 0 additions & 2 deletions sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ private Mono<HttpResponse> attemptRedirect(final HttpPipelineCallContext context

return next.clone().process().flatMap(httpResponse -> {
if (redirectStrategy.shouldAttemptRedirect(context, httpResponse, redirectAttempt, attemptedRedirectUrls)) {

HttpRequest redirectRequestCopy = createRedirectRequest(httpResponse);
return attemptRedirect(context, next, redirectRequestCopy, redirectAttempt + 1, attemptedRedirectUrls);
} else {
Expand All @@ -111,7 +110,6 @@ private HttpResponse attemptRedirectSync(final HttpPipelineCallContext context,
HttpResponse httpResponse = next.clone().processSync();

if (redirectStrategy.shouldAttemptRedirect(context, httpResponse, redirectAttempt, attemptedRedirectUrls)) {

HttpRequest redirectRequestCopy = createRedirectRequest(httpResponse);
return attemptRedirectSync(context, next, redirectRequestCopy, redirectAttempt + 1, attemptedRedirectUrls);
} else {
Expand Down
Loading