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`.
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
|
Hi @vcolin7. 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. |
github-actions
bot
added
the
no-recent-activity
|
Hi @vcolin7. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
Fixes: #39841.
Removed
HttpPipelineNextPolicyandHttpPipelineCallState. RefactoredHttpPipelinePolicyso it knows the next policy in the chain.