Remove HttpPipelineNextPolicy
#40084
Conversation
- Moved some HTTP classes from `http` package to `http.models`. - Renamed `HttpHeader` and `HttpHeaders` to `Header` and `HttpHeader`, respectively. Also moved them to the `models` package. - Removed @deprecated methods. - Removed "Sync" suffix from most classes and methods. - Added necessary classes to `implementation` package.
…n `ExpandableStringEnum`.
…s classes and interfaces in `implementation.serializer`.
…o the `implementation.util` package.
…s, Client Logger methods, sync suffix, API view comments
…it to `HttpRequestOptions`.
…unctionality and state tracking to `HttpPipelinePolicy`.
vcolin7
added
Client
vcolin7
requested review from
samvaity,
JonathanGiles,
srnagar and
alzimmermsft
as code owners
samvaity
force-pushed
the
feature/generic-sdk-core-2
branch
from
32be336
to
c117833
Compare
| * | ||
| * @return A response produced from sending the HTTP request. | ||
| */ | ||
| public Response<?> process(HttpRequest httpRequest, HttpPipeline httpPipeline) { |
There was a problem hiding this comment.
If we're going to be changing the API to remove HttpPipelineNextPolicy I think we should do a major break here instead of trying to retain a similar flow as azure-core. Right now, each pipeline has a mutable state with nextPolicy and HttpPipeline is supposed to be thread-safe which this will break. If two requests are being made where a common RetryPolicy is shared between two pipelines the nextPolicy may not point to the right following policy.
What I'll propose, split process into two methods, onRequest(HttpRequest) : HttpRequest and onResponse(HttpResponse) : HttpResponse. This makes HttpPipelinePolicy a stateless call and removes, what I believe wasn't a great pattern before, where the pipeline policy was both a mutator of requests and responses and managed a small bit of state management as it was forced to call HttpPipelineNextPolicy.process, and if it didn't now it broke the HttpPipeline call.
Simplifying HttpPipelinePolicy to onRequest and onResponse removes state management. It no longer needs to make a call to process the next policy, allowing the HttpPipelinePolicy to be a request and response mutator and HttpPipeline completely owns the state management. This would also allow for policies to be extended into more responsibilities in the future such as onException, if we find additional requirements (though I believe onRequest and onResponse would be sufficient for a long time).
samvaity
requested review from
chenrujun,
Netyyyy,
saragluna,
moarychan,
JimSuplizio,
mssfang,
hallipr,
weshaggard,
benbp and
a team
as code owners
Removed
HttpPipelineNextPolicyandHttpPipelineCallState. RefactoredHttpPipelinePolicyso it knows the next policy in the chain.