Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more #15

Open
wants to merge 1 commit into
base: core2-add-isbuffered
Choose a base branch
from
Open

more #15

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions sdk/core/System.ClientModel/src/Message/PipelineResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,6 @@ public virtual BinaryData Content
// Same value as Stream.CopyTo uses by default
private const int DefaultCopyBufferSize = 81920;

internal bool TryGetBufferedContent(out MemoryStream bufferedContent)
{
if (ContentStream is MemoryStream content)
{
bufferedContent = content;
return true;
}

bufferedContent = default!;
return false;
}

internal void BufferContent(TimeSpan? timeout = default, CancellationTokenSource? cts = default)
=> BufferContentSyncOrAsync(timeout, cts, async: false).EnsureCompleted();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected virtual void Dispose(bool disposing)
{
if (disposing && !_disposed)
{
var httpResponse = _httpResponse;
HttpResponseMessage httpResponse = _httpResponse;
httpResponse?.Dispose();

// Some notes on this:
Expand All @@ -77,19 +77,30 @@ protected virtual void Dispose(bool disposing)
// 2. If the content is not buffered, we dispose it so that we don't leave
// a network connection open.
//
// One tricky piece here is that in some cases, we may not have buffered
// the content because we wanted to pass the live network stream out of
// the client method and back to the end-user caller of the client e.g.
// for a streaming API. If the latter is the case, the client should have
// called the HttpMessage.ExtractResponseContent method to obtain a reference
// to the network stream, and the response content was replaced by a stream
// that we are ok to dispose here. In this case, the network stream is
// not disposed, because the entity that replaced the response content
// intentionally left the network stream undisposed.

var contentStream = _contentStream;
if (contentStream is not null && !TryGetBufferedContent(out _))
// If the response has not been buffered, this is because a client or
// other caller of pipeline.Send set message.BufferResponse = false.
// The reason to do this in a client is because the client is providing
// a streaming API in which it passes the live network stream to the caller
// of a a service method.
//
// In clients with protocol methods, clients must call ExtractResponse
// to allow returning an undisposed response from the protocol method.
// In this case, the caller of the protocol method is responsible for
// disposing the response, which will dispose the network stream with it.
//
// In some Azure.Core-based client convenince methods in libraries without
// protocol methods, the client may have called HttpMessage's
// ExtractResponseContent method to obtain a reference to the network
// stream instead of ExtractResponse. If that is the case, the response
// content was replaced by a stream that makes dispose here a no-op.
// In this case, the caller of the protocol method is also responsible for
// disposing the network stream returned from the convenience method, but
// will do so by disposing the stream directly, rather than disposing the
// response that holds it.

if (!IsBuffered)
{
Stream? contentStream = _contentStream;
contentStream?.Dispose();
_contentStream = null;
}
Expand All @@ -99,4 +110,4 @@ protected virtual void Dispose(bool disposing)
}
#endregion
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,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.
Expand Down Expand Up @@ -136,4 +135,4 @@ internal static void ThrowIfCancellationRequestedOrTimeout(CancellationToken ori
$"The operation was cancelled because it exceeded the configured timeout of {timeout:g}. ");
}
}
}
}