Creating a Redirect Http Policy #23063
Conversation
ghost
added
the
cochi2
force-pushed
the
fmorales/download-sdk-redirection
branch
from
c7ed80b to
fa78f06
Compare
cochi2
requested review from
a team,
JianpingChen,
JonathanGiles,
ankitarorabit,
minnieliu and
mssfang
as code owners
...erver/src/test/java/com/azure/communication/callingserver/DownloadContentAsyncLiveTests.java
Outdated
Show resolved
Hide resolved
| * Location header. | ||
| */ | ||
| public final class RedirectPolicy implements HttpPipelinePolicy { | ||
| private static final int MAX_REDIRECTS = 10; |
There was a problem hiding this comment.
Any concerns around potentially running into a scenario where there are many redirects as well as retries? Since this and the retry policy will both attempt to retry and could result in a multiplicative scenario, ex 3 retry attempts and 10 redirect attempts could result in 30 (possibly 40) network calls before failing.
There was a problem hiding this comment.
Well no, the policy updates the request when doing the redirection. If there is an error, it is the latest version of the request which will retry.
There was a problem hiding this comment.
Good point, I also realized that this is short-circuiting redirect location, so having a request amplification isn't as likely.
There was a problem hiding this comment.
If this is moved to azure-core as suggested above, we should make this customizable (as a constructor arg)
|
|
||
| private boolean shouldRedirect(HttpResponse response, int redirectNumber) { | ||
| return response.getStatusCode() == 302 | ||
| && !locations.contains(response.getHeaderValue(LOCATION_HEADER_NAME)) |
There was a problem hiding this comment.
Based on this a request isn't able to be redirected back to a previously used redirection? Is there a possibility that this causes an error?
There was a problem hiding this comment.
Assuming that the services are deterministic, if we allow to try a previously used location we will end up on some loop. So, we are short circuiting at the first opportunity.
There was a problem hiding this comment.
I think starting with that logic works, another possibility is making the previously used and failed redirects a chain, so if the service attempts to redirect you to somewhere that has been known to redirect can short-circuit the redirection chain isn't of failing the request outright, ex:
location 1 -> 2 -> 3 -> 4 -> 5 -> success
If the redirect is for 3 we could short-circuit to 5. But this introduces a lot more complexity and chances of incorrect behavior.
There was a problem hiding this comment.
We should at least log when a previously attempted redirect link is returned again as the redirect link and reach out to service teams when we notice this behavior. This seems like a bug on the service side that's resulting in a cycle.
...ation-common/src/main/java/com/azure/communication/common/implementation/RedirectPolicy.java
Outdated
Show resolved
Hide resolved
cochi2
force-pushed
the
fmorales/download-sdk-redirection
branch
from
f252251 to
94a831e
Compare
* Replacing List with Set for locations. * Moving locations variable to be a method parameter instead of a class's attribute. * Refactored testing.
cochi2
force-pushed
the
fmorales/download-sdk-redirection
branch
from
94a831e to
3c09f60
Compare
|
Hello @cochi2! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
|
|
||
| private boolean shouldRedirect(HttpResponse response, int redirectNumber) { | ||
| return response.getStatusCode() == 302 | ||
| && !locations.contains(response.getHeaderValue(LOCATION_HEADER_NAME)) |
There was a problem hiding this comment.
We should at least log when a previously attempted redirect link is returned again as the redirect link and reach out to service teams when we notice this behavior. This seems like a bug on the service side that's resulting in a cycle.
| * HttpPipelinePolicy to redirect requests when 302 message is received to the new location marked by the | ||
| * Location header. | ||
| */ | ||
| public final class RedirectPolicy implements HttpPipelinePolicy { |
There was a problem hiding this comment.
This policy is a duplicate of what we have in another service - azure-monitor-opentelemetry-exporter. We should move this to azure-core, so that both services (and potentially others) can share.
| }); | ||
| } | ||
|
|
||
| private boolean shouldRedirect(HttpResponse response, int redirectNumber, Set<String> locations) { |
There was a problem hiding this comment.
nit: name redirectNumber as redirectAttempt or tryCount.
| } | ||
|
|
||
| private Mono<HttpResponse> attemptRedirection(HttpPipelineCallContext context, HttpPipelineNextPolicy next, | ||
| int redirectNumber, Set<String> locations) { |
There was a problem hiding this comment.
name locations as attemptedRedirectLocations
| * Location header. | ||
| */ | ||
| public final class RedirectPolicy implements HttpPipelinePolicy { | ||
| private static final int MAX_REDIRECTS = 10; |
There was a problem hiding this comment.
If this is moved to azure-core as suggested above, we should make this customizable (as a constructor arg)
| } | ||
|
|
||
| private boolean shouldRedirect(HttpResponse response, int redirectNumber, Set<String> locations) { | ||
| return response.getStatusCode() == 302 |
There was a problem hiding this comment.
What about other status codes like 301 (moved permanently), 307 (temporary redirect) and 308 (permanent redirect)? Also, instead of using magic numbers, use named constants.
Creating a new http policy that, if added, will redirect any request, upon reception of an 302 Http Response, to the new URL received under the "Location" header.