ClientModel Prototype: Don't keep MemoryStream buffer on PipelineResponse

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

@@ -239,7 +239,6 @@ public abstract partial class Response : System.ClientModel.Primitives.PipelineR
     {
         protected Response() { }
         public abstract string ClientRequestId { get; set; }
-        public virtual new System.BinaryData Content { get { throw null; } }
         public virtual new Azure.Core.ResponseHeaders Headers { get { throw null; } }
         protected internal abstract bool ContainsHeader(string name);
         protected internal abstract System.Collections.Generic.IEnumerable<Azure.Core.HttpHeader> EnumerateHeaders();

sdk/core/Azure.Core/api/Azure.Core.net472.cs

@@ -239,7 +239,6 @@ public abstract partial class Response : System.ClientModel.Primitives.PipelineR
     {
         protected Response() { }
         public abstract string ClientRequestId { get; set; }
-        public virtual new System.BinaryData Content { get { throw null; } }
         public virtual new Azure.Core.ResponseHeaders Headers { get { throw null; } }
         protected internal abstract bool ContainsHeader(string name);
         protected internal abstract System.Collections.Generic.IEnumerable<Azure.Core.HttpHeader> EnumerateHeaders();

sdk/core/Azure.Core/api/Azure.Core.net6.0.cs

@@ -239,7 +239,6 @@ public abstract partial class Response : System.ClientModel.Primitives.PipelineR
     {
         protected Response() { }
         public abstract string ClientRequestId { get; set; }
-        public virtual new System.BinaryData Content { get { throw null; } }
         public virtual new Azure.Core.ResponseHeaders Headers { get { throw null; } }
         protected internal abstract bool ContainsHeader(string name);
         protected internal abstract System.Collections.Generic.IEnumerable<Azure.Core.HttpHeader> EnumerateHeaders();

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

@@ -239,7 +239,6 @@ public abstract partial class Response : System.ClientModel.Primitives.PipelineR
     {
         protected Response() { }
         public abstract string ClientRequestId { get; set; }
-        public virtual new System.BinaryData Content { get { throw null; } }
         public virtual new Azure.Core.ResponseHeaders Headers { get { throw null; } }
         protected internal abstract bool ContainsHeader(string name);
         protected internal abstract System.Collections.Generic.IEnumerable<Azure.Core.HttpHeader> EnumerateHeaders();

sdk/core/Azure.Core/src/Response.cs

@@ -29,14 +29,6 @@ public abstract class Response : PipelineResponse
         // TODO: is is possible to not new-slot this?
         public new virtual ResponseHeaders Headers => new ResponseHeaders(this);
 
-        /// <summary>
-        /// Gets the contents of HTTP response, if it is available.
-        /// </summary>
-        /// <remarks>
-        /// Throws <see cref="InvalidOperationException"/> when <see cref="PipelineResponse.ContentStream"/> is not a <see cref="MemoryStream"/>.
-        /// </remarks>
-        public new virtual BinaryData Content => base.Content;
-
         /// <summary>
         /// TBD.
         /// </summary>

sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs

@@ -197,8 +197,8 @@ protected PipelineRequest() { }
     public abstract partial class PipelineResponse : System.IDisposable
     {
         protected PipelineResponse() { }
-        public System.BinaryData Content { get { throw null; } }
-        public abstract System.IO.Stream? ContentStream { get; set; }
+        public virtual System.BinaryData Content { get { throw null; } }
+        public virtual System.IO.Stream? ContentStream { get { throw null; } set { } }
         public System.ClientModel.Primitives.MessageHeaders Headers { get { throw null; } }
         public virtual bool IsError { get { throw null; } }
         public abstract string ReasonPhrase { get; }

sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs

@@ -196,8 +196,8 @@ protected PipelineRequest() { }
     public abstract partial class PipelineResponse : System.IDisposable
     {
         protected PipelineResponse() { }
-        public System.BinaryData Content { get { throw null; } }
-        public abstract System.IO.Stream? ContentStream { get; set; }
+        public virtual System.BinaryData Content { get { throw null; } }
+        public virtual System.IO.Stream? ContentStream { get { throw null; } set { } }
         public System.ClientModel.Primitives.MessageHeaders Headers { get { throw null; } }
         public virtual bool IsError { get { throw null; } }
         public abstract string ReasonPhrase { get; }

sdk/core/System.ClientModel/src/Message/PipelineResponse.cs

@@ -19,6 +19,8 @@ public abstract class PipelineResponse : IDisposable
 
     private bool _isError = false;
 
+    private BinaryData? _content;
+
     /// <summary>
     /// Gets the HTTP status code.
     /// </summary>
@@ -38,30 +40,21 @@ public abstract class PipelineResponse : IDisposable
     /// </summary>
     public abstract Stream? ContentStream { get; set; }
 
-    #region Meta-data properties set by the pipeline.
-
-    public BinaryData Content
+    public virtual BinaryData Content
     {
         get
         {
-            if (ContentStream == null)
+            if (_contentStream is not null)
             {
-                return s_emptyBinaryData;
+                return BinaryData.FromStream(_contentStream);
             }
 
-            if (!TryGetBufferedContent(out MemoryStream bufferedContent))
+            if (_content is null)
             {
-                throw new InvalidOperationException($"The response is not buffered.");
+                throw new InvalidOperationException($"The response is not fully buffered.");
             }
 
-            if (bufferedContent.TryGetBuffer(out ArraySegment<byte> segment))
-            {
-                return new BinaryData(segment.AsMemory());
-            }
-            else
-            {
-                return new BinaryData(bufferedContent.ToArray());
-            }
+            return _content;
         }
     }
 
@@ -80,7 +73,7 @@ public BinaryData Content
 
     internal TimeSpan NetworkTimeout { get; set; } = DefaultNetworkTimeout;
 
-    #endregion
+    internal bool IsBuffered { get; private set; }
 
     public abstract void Dispose();
 
@@ -89,48 +82,58 @@ public BinaryData Content
     // Same value as Stream.CopyTo uses by default
     private const int DefaultCopyBufferSize = 81920;
 
-    internal bool TryGetBufferedContent(out MemoryStream bufferedContent)
+    internal void BufferContent(TimeSpan? timeout = default, CancellationTokenSource? cts = default)
     {
-        if (ContentStream is MemoryStream content)
+        if (IsBuffered)
         {
-            bufferedContent = content;
-            return true;
+            return;
         }
 
-        bufferedContent = default!;
-        return false;
-    }
-
-    internal void BufferContent(TimeSpan? timeout = default, CancellationTokenSource? cts = default)
-    {
         Stream? responseContentStream = ContentStream;
-        if (responseContentStream == null || TryGetBufferedContent(out _))
+        if (responseContentStream == null)
         {
-            // No need to buffer content.
+            _content = s_emptyBinaryData;
             return;
         }
 
         MemoryStream bufferStream = new();
         CopyTo(responseContentStream, bufferStream, timeout ?? NetworkTimeout, cts ?? new CancellationTokenSource());
+
+        // Dispose the network stream.
         responseContentStream.Dispose();
-        bufferStream.Position = 0;
-        ContentStream = bufferStream;
+
+        _content = bufferStream.TryGetBuffer(out ArraySegment<byte> segment) ?
+            new BinaryData(segment.AsMemory()) :
+            new BinaryData(bufferStream.ToArray());
+
+        IsBuffered = true;
     }
 
     internal async Task BufferContentAsync(TimeSpan? timeout = default, CancellationTokenSource? cts = default)
     {
+        if (IsBuffered)
+        {
+            return;
+        }
+
         Stream? responseContentStream = ContentStream;
-        if (responseContentStream == null || TryGetBufferedContent(out _))
+        if (responseContentStream == null)
         {
-            // No need to buffer content.
+            _content = s_emptyBinaryData;
             return;
         }
 
         MemoryStream bufferStream = new();
         await CopyToAsync(responseContentStream, bufferStream, timeout ?? NetworkTimeout, cts ?? new CancellationTokenSource()).ConfigureAwait(false);
+
+        // Dispose the network stream.
         responseContentStream.Dispose();
-        bufferStream.Position = 0;
-        ContentStream = bufferStream;
+
+        _content = bufferStream.TryGetBuffer(out ArraySegment<byte> segment) ?
+            new BinaryData(segment.AsMemory()) :
+            new BinaryData(bufferStream.ToArray());
+
+        IsBuffered = true;
     }
 
     private static async Task CopyToAsync(Stream source, Stream destination, TimeSpan timeout, CancellationTokenSource cancellationTokenSource)

sdk/core/System.ClientModel/src/Pipeline/HttpClientPipelineTransport.Response.cs

@@ -88,7 +88,7 @@ protected virtual void Dispose(bool disposing)
                 // intentionally left the network stream undisposed.
 
                 var contentStream = _contentStream;
-                if (contentStream is not null && !TryGetBufferedContent(out _))
+                if (contentStream is not null && !IsBuffered)
                 {
                     contentStream?.Dispose();
                     _contentStream = null;

sdk/core/System.ClientModel/src/Pipeline/HttpClientPipelineTransport.cs

@@ -160,13 +160,6 @@ private async ValueTask ProcessSyncOrAsync(PipelineMessage message, bool async)
         //   2. Make any necessary modifications based on the System.Net.Http.HttpResponseMessage.
         OnReceivedResponse(message, responseMessage);
 
-        // We set derived values on the MessageResponse here, including Content and IsError
-        // to ensure these things happen in the transport.  If derived implementations need
-        // to override these default transport values, they can do so in pipeline policies.
-
-        // TODO: a possible alternative is to make instantiating the Response a specific
-        // extensibilty point and let OnReceivedResponse enable transport-specific logic.
-        // Consider which is preferred as part of holistic extensibility-point review.
         if (contentStream is not null)
         {
             message.Response.ContentStream = contentStream;

sdk/core/System.ClientModel/src/Pipeline/ResponseBufferingPolicy.cs

@@ -89,8 +89,7 @@ private async ValueTask ProcessSyncOrAsync(PipelineMessage message, IReadOnlyLis
         message.Response!.NetworkTimeout = invocationNetworkTimeout;
 
         Stream? responseContentStream = message.Response!.ContentStream;
-        if (responseContentStream is null ||
-            message.Response.TryGetBufferedContent(out var _))
+        if (responseContentStream is null || message.Response.IsBuffered)
         {
             // There is either no content on the response, or the content has already
             // been buffered.