Azure.Core 2.0: Update HttpPipelinePolicy, RequestFailedException, and transport types#42294
Conversation
| Debug.Assert(pipeline.IsEmpty); | ||
|
|
||
| await _transport.ProcessAsync(message).ConfigureAwait(false); | ||
| await _transport.ProcessAsync(message as PipelineMessage).ConfigureAwait(false); |
There was a problem hiding this comment.
This calls through to the base PipelineTransport implementation that handles the response buffering now.
annelo-msft
requested review from
a team,
KrzysztofCwalina and
tg-msft
as code owners
annelo-msft
requested review from
AlexanderSher,
JoshLove-msft,
christothes,
jsquire and
pallavit
| { | ||
| _responseMessage?.Dispose(); | ||
| DisposeStreamIfNotBuffered(ref _contentStream); | ||
| if (response.ContentStream is MemoryStream && |
There was a problem hiding this comment.
Is the MemoryStream type necessary here, or are we including it just to avoid any behavioral changes?
There was a problem hiding this comment.
It's required because of the current behavior of HttpMessage.ExtractResponseContent -- it replaces ContentStream with a stream that throws if you call CanSeek on it. Checking MemoryStream limits the check to cases where either the transport response type did the buffering or a unit test replaced the ContentStream with something custom.
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.ClientModel; |
There was a problem hiding this comment.
There seems to be a lot of complex networking code here. I was expecting that all this code would be in client model and then Azure.Core would have a very simple call forwarding adapter. If we cannot get to that point, is there a good reason for all this inheritance or we should have two separate pipelines?
There was a problem hiding this comment.
Are you referring to lines 126-224? This is for cookie, proxy, and cert handling that I had assumed were specific to Azure clients. If we feel they are general enough to move into ClientModel, we can do that. The cookie we have is named "Azure.Core.Pipeline.HttpClientTransport.EnableCookies" so we would probably want a different name for that one.
There was a problem hiding this comment.
It's not any single part of the code. It's the overall feeling that the resulting Azure.Core is now even harder to understand than it was before. Possibly I am wrong about this, but I want to make sure we don't end up with a mess in Azure.Core.
| /// Represent an extension point for the <see cref="HttpPipeline"/> that can mutate the <see cref="Request"/> and react to received <see cref="Response"/>. | ||
| /// </summary> | ||
| public abstract class HttpPipelinePolicy | ||
| public abstract class HttpPipelinePolicy : PipelinePolicy |
There was a problem hiding this comment.
Since we are doing adapters, is the inheritance needed?
There was a problem hiding this comment.
The ClientModel pipeline needs to be able to call ProcessNext on an Azure.Core policy from a System.ClientModel policy. We can look at another way of doing this, but inheritance allows us to do this rather simply.
|
I've added the inheritance for RequestFailedException to this PR since there is a Search client test that relies on the implementation of RFE that uses the new buffering logic from ClientModel. It will bring some DigitalTwins dependencies with it, as those have refdocs that rely on a property that has moved to ClientModel. |
annelo-msft
requested review from
Aashish93-stack,
abhinav-ghai,
johngallardo and
sjiherzig
as code owners
| { | ||
| _response = response; | ||
| } | ||
| #region ISerializable implementation |
There was a problem hiding this comment.
For example, @KrzysztofCwalina, we could remove the serialization constructor from the base exception type in ClientModel if we wanted to, and add it back if/when we did the Azure.Core 2.0 integration.
annelo-msft
changed the title
|
Discussed offline with @KrzysztofCwalina that it is ok to merge this for now -- we can revisit our approach here separately. |
4 participants
Description
Many of the basic Azure.Core types now have analogous types in the System.ClientModel library. In order to follow the DRY engineering principle, we plan to update Azure.Core to reuse functionality from these types, using either inheritance or adapters in most cases.
This PR makes the following changes:
HttpPipelinePolicyinherits from System.ClientModelPipelinePolicyHttpPipelineTransportinherits from System.ClientModelPipelineTransportHttpClientTransportadapts System.ClientModelHttpClientPipelineTransportPipelineTransportand so is able to remove internalResponseBodyPolicyfrom Azure.Core. As part of this, moves NetworkTimeout value to be a property ofHttpPipeline.RequestFailedExceptioninherits from System.ClientModelClientRequestExceptionDesign Motivation
This one is the most complex of the integration pieces. Because System.ClientModel
HttpClientPipelineTransportrequest and response implementations are private, we have to call through toHttpClientPipelineTransport.CreateMessageto get an instance of the request. This means fields backing theRequestproperties are duplicated across the adaptedPipelineRequestinstance and the virtual properties on the baseRequesttype.Context
This PR is a smaller chunk of #41773 to make it easier to review. In addition, it this PR targets the feature/azure.core-2.0 feature branch because we cannot move the Azure.Core 2.0 integration into main until after System.ClientModel 1.1.0 is GA.