Skip to content

Commit 240c23f

Browse files
christotheschristothes
authored
Azure.Core API feedback (Azure#26154)
* API feedback
1 parent a590b68 commit 240c23f

File tree

12 files changed

+114
-64
lines changed

12 files changed

+114
-64
lines changed

‎sdk/confidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerClient.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ public ConfidentialLedgerClient(Uri ledgerUri, TokenCredential credential, Confi
4040
actualOptions,
4141
Array.Empty<HttpPipelinePolicy>(),
4242
new HttpPipelinePolicy[] { authPolicy },
43-
new ResponseClassifier(),
44-
transportOptions);
43+
transportOptions,
44+
new ResponseClassifier());
4545
_ledgerUri = ledgerUri;
4646
_apiVersion = actualOptions.Version;
4747
}

‎sdk/core/Azure.Core/api/Azure.Core.net461.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,7 @@ public HttpClientTransport() { }
757757
public HttpClientTransport(System.Net.Http.HttpClient client) { }
758758
public HttpClientTransport(System.Net.Http.HttpMessageHandler messageHandler) { }
759759
public sealed override Azure.Core.Request CreateRequest() { throw null; }
760+
public void Dispose() { }
760761
public override void Process(Azure.Core.HttpMessage message) { }
761762
public override System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message) { throw null; }
762763
}
@@ -777,8 +778,8 @@ public void Send(Azure.Core.HttpMessage message, System.Threading.CancellationTo
777778
public static partial class HttpPipelineBuilder
778779
{
779780
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, params Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies) { throw null; }
781+
public static Azure.Core.Pipeline.DisposableHttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.Pipeline.HttpPipelineTransportOptions transportOptions, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
780782
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
781-
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier, Azure.Core.Pipeline.HttpPipelineTransportOptions? defaultTransportOptions) { throw null; }
782783
}
783784
public abstract partial class HttpPipelinePolicy
784785
{
@@ -800,7 +801,6 @@ public abstract partial class HttpPipelineTransport
800801
{
801802
protected HttpPipelineTransport() { }
802803
public abstract Azure.Core.Request CreateRequest();
803-
public void Dispose() { }
804804
public abstract void Process(Azure.Core.HttpMessage message);
805805
public abstract System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message);
806806
}

‎sdk/core/Azure.Core/api/Azure.Core.net5.0.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,7 @@ public HttpClientTransport() { }
757757
public HttpClientTransport(System.Net.Http.HttpClient client) { }
758758
public HttpClientTransport(System.Net.Http.HttpMessageHandler messageHandler) { }
759759
public sealed override Azure.Core.Request CreateRequest() { throw null; }
760+
public void Dispose() { }
760761
public override void Process(Azure.Core.HttpMessage message) { }
761762
public override System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message) { throw null; }
762763
}
@@ -777,8 +778,8 @@ public void Send(Azure.Core.HttpMessage message, System.Threading.CancellationTo
777778
public static partial class HttpPipelineBuilder
778779
{
779780
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, params Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies) { throw null; }
781+
public static Azure.Core.Pipeline.DisposableHttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.Pipeline.HttpPipelineTransportOptions transportOptions, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
780782
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
781-
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier, Azure.Core.Pipeline.HttpPipelineTransportOptions? defaultTransportOptions) { throw null; }
782783
}
783784
public abstract partial class HttpPipelinePolicy
784785
{
@@ -800,7 +801,6 @@ public abstract partial class HttpPipelineTransport
800801
{
801802
protected HttpPipelineTransport() { }
802803
public abstract Azure.Core.Request CreateRequest();
803-
public void Dispose() { }
804804
public abstract void Process(Azure.Core.HttpMessage message);
805805
public abstract System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message);
806806
}

‎sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,7 @@ public HttpClientTransport() { }
757757
public HttpClientTransport(System.Net.Http.HttpClient client) { }
758758
public HttpClientTransport(System.Net.Http.HttpMessageHandler messageHandler) { }
759759
public sealed override Azure.Core.Request CreateRequest() { throw null; }
760+
public void Dispose() { }
760761
public override void Process(Azure.Core.HttpMessage message) { }
761762
public override System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message) { throw null; }
762763
}
@@ -777,8 +778,8 @@ public void Send(Azure.Core.HttpMessage message, System.Threading.CancellationTo
777778
public static partial class HttpPipelineBuilder
778779
{
779780
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, params Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies) { throw null; }
781+
public static Azure.Core.Pipeline.DisposableHttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.Pipeline.HttpPipelineTransportOptions transportOptions, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
780782
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
781-
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier, Azure.Core.Pipeline.HttpPipelineTransportOptions? defaultTransportOptions) { throw null; }
782783
}
783784
public abstract partial class HttpPipelinePolicy
784785
{
@@ -800,7 +801,6 @@ public abstract partial class HttpPipelineTransport
800801
{
801802
protected HttpPipelineTransport() { }
802803
public abstract Azure.Core.Request CreateRequest();
803-
public void Dispose() { }
804804
public abstract void Process(Azure.Core.HttpMessage message);
805805
public abstract System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message);
806806
}

‎sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,7 @@ public HttpClientTransport() { }
757757
public HttpClientTransport(System.Net.Http.HttpClient client) { }
758758
public HttpClientTransport(System.Net.Http.HttpMessageHandler messageHandler) { }
759759
public sealed override Azure.Core.Request CreateRequest() { throw null; }
760+
public void Dispose() { }
760761
public override void Process(Azure.Core.HttpMessage message) { }
761762
public override System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message) { throw null; }
762763
}
@@ -777,8 +778,8 @@ public void Send(Azure.Core.HttpMessage message, System.Threading.CancellationTo
777778
public static partial class HttpPipelineBuilder
778779
{
779780
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, params Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies) { throw null; }
781+
public static Azure.Core.Pipeline.DisposableHttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.Pipeline.HttpPipelineTransportOptions transportOptions, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
780782
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier) { throw null; }
781-
public static Azure.Core.Pipeline.HttpPipeline Build(Azure.Core.ClientOptions options, Azure.Core.Pipeline.HttpPipelinePolicy[] perCallPolicies, Azure.Core.Pipeline.HttpPipelinePolicy[] perRetryPolicies, Azure.Core.ResponseClassifier? responseClassifier, Azure.Core.Pipeline.HttpPipelineTransportOptions? defaultTransportOptions) { throw null; }
782783
}
783784
public abstract partial class HttpPipelinePolicy
784785
{
@@ -800,7 +801,6 @@ public abstract partial class HttpPipelineTransport
800801
{
801802
protected HttpPipelineTransport() { }
802803
public abstract Azure.Core.Request CreateRequest();
803-
public void Dispose() { }
804804
public abstract void Process(Azure.Core.HttpMessage message);
805805
public abstract System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message);
806806
}

‎sdk/core/Azure.Core/src/Pipeline/DisposableHttpPipeline.cs‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,15 @@
66
namespace Azure.Core.Pipeline
77
{
88
/// <summary>
9-
/// A wrapper type for <see cref="HttpPipeline"/> that exposes a <see cref="Dispose"/> method for disposal of the underlying <see cref="HttpPipelineTransport"/>.
9+
/// An implementation of <see cref="HttpPipeline"/> that may contain resources that require disposal.
1010
/// </summary>
1111
public sealed class DisposableHttpPipeline : HttpPipeline, IDisposable
1212
{
13+
/// <summary>
14+
/// Indicates whether this instance was constructed internally. This information is used to determine if Dispose(true) is required to dispose of the Transport.
15+
/// </summary>
16+
private bool isTransportOwnedInternally;
17+
1318
/// <summary>
1419
/// Creates a new instance of <see cref="HttpPipeline"/> with the provided transport, policies and response classifier.
1520
/// </summary>
@@ -18,17 +23,28 @@ public sealed class DisposableHttpPipeline : HttpPipeline, IDisposable
1823
/// <param name="perRetryIndex"></param>
1924
/// <param name="policies">Policies to be invoked as part of the pipeline in order.</param>
2025
/// <param name="responseClassifier">The response classifier to be used in invocations.</param>
21-
internal DisposableHttpPipeline(HttpPipelineTransport transport, int perCallIndex, int perRetryIndex, HttpPipelinePolicy[] policies, ResponseClassifier responseClassifier)
26+
/// <param name="isTransportOwnedInternally"> </param>
27+
internal DisposableHttpPipeline(HttpPipelineTransport transport, int perCallIndex, int perRetryIndex, HttpPipelinePolicy[] policies, ResponseClassifier responseClassifier, bool isTransportOwnedInternally)
2228
: base(transport, perCallIndex, perRetryIndex, policies, responseClassifier)
2329
{
30+
this.isTransportOwnedInternally = isTransportOwnedInternally;
2431
}
2532

2633
/// <summary>
27-
/// Calls Dispose on the underlying <see cref="HttpPipelineTransport"/>.
34+
/// Disposes the underlying transport if it is owned by the client, i.e. it was created via the Build method on <see cref="HttpPipelineBuilder"/>. If the underlying transport is not owned by the client, i.e. it was supplied as a custom transport on <see cref="ClientOptions"/>, it will not be disposed.
35+
/// <remarks>
36+
/// The reason not to dispose a transport owned outside the client, i.e. one that was provided via <see cref="ClientOptions"/> is to account for scenarios
37+
/// where the custom transport may be shared across clients. In this case, it is possible to dispose of a transport
38+
/// still in use by other clients. When the transport is created internally, it can properly determine if a shared instance is in use.
39+
/// </remarks>
2840
/// </summary>
2941
public void Dispose()
3042
{
31-
_transport.Dispose();
43+
// TODO: When transport disposal is needed for a nested client scenario, this method should implement reference counting to ensure proper disposal.
44+
if (isTransportOwnedInternally)
45+
{
46+
(_transport as IDisposable)?.Dispose();
47+
}
3248
}
3349
}
3450
}

‎sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -593,12 +593,16 @@ private static HttpClientHandler ApplyOptionsToHandler(HttpClientHandler httpHan
593593
return httpHandler;
594594
}
595595

596-
internal override void DisposeInternal()
596+
/// <summary>
597+
/// Disposes the underlying <see cref="HttpClient"/>.
598+
/// </summary>
599+
public void Dispose()
597600
{
598601
if (this != Shared)
599602
{
600603
Client.Dispose();
601604
}
605+
GC.SuppressFinalize(this);
602606
}
603607

604608
private static void SetPropertiesOrOptions<T>(HttpRequestMessage httpRequest, string name, T value)

‎sdk/core/Azure.Core/src/Pipeline/HttpPipelineBuilder.cs‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Linq;
77
using System.Reflection;
8+
using System.Threading;
89
using Azure.Core.Diagnostics;
910

1011
namespace Azure.Core.Pipeline
@@ -39,19 +40,32 @@ public static HttpPipeline Build(
3940
HttpPipelinePolicy[] perRetryPolicies,
4041
ResponseClassifier? responseClassifier)
4142
{
42-
return Build(options, perCallPolicies, perRetryPolicies, responseClassifier, null);
43+
var result = BuildInternal(options, perCallPolicies, perRetryPolicies, null, responseClassifier);
44+
return new HttpPipeline(result.Transport, result.PerCallIndex, result.PerRetryIndex, result.Policies, result.Classifier);
4345
}
4446

4547
/// <summary>
46-
/// Creates an instance of <see cref="HttpPipeline"/> populated with default policies, customer provided policies from <paramref name="options"/> and client provided per call policies.
48+
/// Creates an instance of <see cref="DisposableHttpPipeline"/> populated with default policies, customer provided policies from <paramref name="options"/>, client provided per call policies, and the supplied <see cref="HttpPipelineTransportOptions"/>.
4749
/// </summary>
4850
/// <param name="options">The customer provided client options object.</param>
4951
/// <param name="perCallPolicies">Client provided per-call policies.</param>
5052
/// <param name="perRetryPolicies">Client provided per-retry policies.</param>
53+
/// <param name="transportOptions">The customer provided transport options which will be applied to the default transport. Note: If a custom transport has been supplied via the <paramref name="options"/>, these <paramref name="transportOptions"/> will be ignored.</param>
5154
/// <param name="responseClassifier">The client provided response classifier.</param>
52-
/// <param name="defaultTransportOptions">The customer provided transport options which will be applied to the default transport.</param>
53-
/// <returns>A new instance of <see cref="HttpPipeline"/></returns>
54-
public static HttpPipeline Build(ClientOptions options, HttpPipelinePolicy[] perCallPolicies, HttpPipelinePolicy[] perRetryPolicies, ResponseClassifier? responseClassifier, HttpPipelineTransportOptions? defaultTransportOptions)
55+
/// <returns>A new instance of <see cref="DisposableHttpPipeline"/></returns>
56+
public static DisposableHttpPipeline Build(ClientOptions options, HttpPipelinePolicy[] perCallPolicies, HttpPipelinePolicy[] perRetryPolicies, HttpPipelineTransportOptions transportOptions, ResponseClassifier? responseClassifier)
57+
{
58+
Argument.AssertNotNull(transportOptions, nameof(transportOptions));
59+
var result = BuildInternal(options, perCallPolicies, perRetryPolicies, transportOptions, responseClassifier);
60+
return new DisposableHttpPipeline(result.Transport, result.PerCallIndex, result.PerRetryIndex, result.Policies, result.Classifier, result.IsTransportOwned);
61+
}
62+
63+
internal static (ResponseClassifier Classifier, HttpPipelineTransport Transport, int PerCallIndex, int PerRetryIndex, HttpPipelinePolicy[] Policies, bool IsTransportOwned) BuildInternal(
64+
ClientOptions options,
65+
HttpPipelinePolicy[] perCallPolicies,
66+
HttpPipelinePolicy[] perRetryPolicies,
67+
HttpPipelineTransportOptions? defaultTransportOptions,
68+
ResponseClassifier? responseClassifier)
5569
{
5670
if (perCallPolicies == null)
5771
{
@@ -133,7 +147,8 @@ void AddCustomerPolicies(HttpPipelinePosition position)
133147

134148
// Override the provided Transport with the provided transport options if the transport has not been set after default construction and options are not null.
135149
HttpPipelineTransport transport = options.Transport;
136-
bool disposablePipeline = false;
150+
bool isTransportInternallyCreated = false;
151+
137152
if (defaultTransportOptions != null)
138153
{
139154
if (options.IsCustomTransportSet)
@@ -147,27 +162,15 @@ void AddCustomerPolicies(HttpPipelinePosition position)
147162
else
148163
{
149164
transport = HttpPipelineTransport.Create(defaultTransportOptions);
165+
isTransportInternallyCreated = true;
150166
}
151167
}
152168

153169
policies.Add(new HttpPipelineTransportPolicy(transport, sanitizer));
154170

155171
responseClassifier ??= ResponseClassifier.Shared;
156172

157-
if (disposablePipeline)
158-
{
159-
return new DisposableHttpPipeline(transport,
160-
perCallIndex,
161-
perRetryIndex,
162-
policies.ToArray(),
163-
responseClassifier);
164-
}
165-
166-
return new HttpPipeline(transport,
167-
perCallIndex,
168-
perRetryIndex,
169-
policies.ToArray(),
170-
responseClassifier);
173+
return (responseClassifier, transport, perCallIndex, perRetryIndex, policies.ToArray(), isTransportInternallyCreated);
171174
}
172175

173176
// internal for testing

‎sdk/core/Azure.Core/src/Pipeline/HttpPipelineTransport.cs‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -55,20 +55,5 @@ internal static HttpPipelineTransport Create(HttpPipelineTransportOptions? optio
5555
_ => new HttpClientTransport(options)
5656
};
5757
}
58-
59-
/// <summary>
60-
/// Disposes the underlying transport.
61-
/// </summary>
62-
public void Dispose()
63-
{
64-
// TODO: When transport disposal is needed for a nested client scenario, this method should implement reference counting to ensure proper disposal.
65-
DisposeInternal();
66-
}
67-
68-
/// <summary>
69-
/// Dispose implementation for implementors to override for freeing resources on Dispose.
70-
/// This method should not be called directly.
71-
/// </summary>
72-
internal virtual void DisposeInternal() { }
7358
}
7459
}

0 commit comments

Comments
 (0)