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

Null check bug fixes and tweak accessibility of properties on TransportResponse #144

Merged
merged 6 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 1 addition & 3 deletions src/Elastic.Transport/Responses/DefaultResponseFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,11 @@ private async ValueTask<TResponse> CreateCoreAsync<TResponse>(
IReadOnlyDictionary<TcpState, int>? tcpStats,
CancellationToken cancellationToken = default) where TResponse : TransportResponse, new()
{
responseStream.ThrowIfNull(nameof(responseStream));

var details = InitializeApiCallDetails(endpoint, boundConfiguration, postData, ex, statusCode, headers, contentType, threadPoolStats, tcpStats, contentLength);

TResponse? response = null;

if (MayHaveBody(statusCode, endpoint.Method, contentLength)
if (responseStream is not null && MayHaveBody(statusCode, endpoint.Method, contentLength)
&& TryResolveBuilder<TResponse>(boundConfiguration.ResponseBuilders, boundConfiguration.ProductResponseBuilders, out var builder))
{
var ownsStream = false;
Expand Down
56 changes: 11 additions & 45 deletions src/Elastic.Transport/Responses/Special/StreamResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,65 +8,31 @@
namespace Elastic.Transport;

/// <summary>
/// A response that exposes the response <see cref="TransportResponse{T}.Body"/> as <see cref="Stream"/>.
/// A response that exposes the response as a <see cref="Stream"/>.
/// <para>
/// <strong>MUST</strong> be disposed after use to ensure the HTTP connection is freed for reuse.
/// </para>
/// </summary>
public class StreamResponse : TransportResponse<Stream>, IDisposable
public sealed class StreamResponse : StreamResponseBase, IDisposable
{
private bool _disposed;

/// <summary>
/// The MIME type of the response, if present.
/// </summary>
public string ContentType { get; }

/// <inheritdoc cref="StreamResponse"/>
public StreamResponse()
{
Body = Stream.Null;
public StreamResponse() : base(Stream.Null) =>
ContentType = string.Empty;
}

/// <inheritdoc cref="StreamResponse"/>
public StreamResponse(Stream body, string? contentType)
{
Body = body;
public StreamResponse(Stream body, string? contentType) : base(body) =>
ContentType = contentType ?? string.Empty;
}

internal override bool LeaveOpen => true;

/// <summary>
/// Disposes the underlying stream.
/// The MIME type of the response, if present.
/// </summary>
/// <param name="disposing"></param>
protected virtual void Dispose(bool disposing)
{
if (!_disposed)
{
if (disposing)
{
Body.Dispose();

if (LinkedDisposables is not null)
{
foreach (var disposable in LinkedDisposables)
disposable.Dispose();
}
}

_disposed = true;
}
}
public string ContentType { get; }

/// <summary>
/// Disposes the underlying stream.
/// The raw response stream.
/// </summary>
public void Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
public Stream Body => Stream;

/// <inheritdoc/>
protected internal override bool LeaveOpen => true;
}
62 changes: 62 additions & 0 deletions src/Elastic.Transport/Responses/Special/StreamResponseBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Licensed to Elasticsearch B.V under one or more agreements.
// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information

using System;
using System.IO;

namespace Elastic.Transport;

/// <summary>
/// A base class for implementing responses that access the raw response stream.
/// </summary>
public abstract class StreamResponseBase(Stream stream) : TransportResponse, IDisposable
stevejgordon marked this conversation as resolved.
Show resolved Hide resolved
{
/// <inheritdoc/>
protected internal override bool LeaveOpen => true;

/// <summary>
/// The raw response stream from the HTTP layer.
/// </summary>
/// <remarks>
/// <b>MUST</b> be disposed to release the underlying HTTP connection for reuse.
/// </remarks>
protected Stream Stream { get; } = stream;

/// <summary>
/// Indicates that the response has been disposed and it is not longer safe to access the stream.
/// </summary>
protected bool Disposed { get; private set; }

/// <summary>
/// Disposes the underlying stream.
/// </summary>
/// <param name="disposing"></param>
protected virtual void Dispose(bool disposing)
{
if (!Disposed)
{
if (disposing)
{
Stream?.Dispose();

if (LinkedDisposables is not null)
{
foreach (var disposable in LinkedDisposables)
disposable?.Dispose();
}
}

Disposed = true;
}
}

/// <summary>
/// Disposes the underlying stream.
/// </summary>
public void Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
}
8 changes: 4 additions & 4 deletions src/Elastic.Transport/Responses/TransportResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ namespace Elastic.Transport;

/// <summary>
/// A response from an Elastic product including details about the request/response life cycle. Base class for the built in low level response
/// types, <see cref="StringResponse"/>, <see cref="BytesResponse"/>, <see cref="DynamicResponse"/>, <see cref="StreamResponse"/> and <see cref="VoidResponse"/>
/// types, <see cref="StringResponse"/>, <see cref="BytesResponse"/>, <see cref="DynamicResponse"/>, and <see cref="VoidResponse"/>
/// </summary>
public abstract class TransportResponse<T> : TransportResponse
{
/// <summary>
/// The deserialized body returned by the product.
/// The (potentially deserialized) response returned by the product.
/// </summary>
public T Body { get; protected internal set; }
}
Expand Down Expand Up @@ -46,7 +46,7 @@ public override string ToString() => ApiCallDetails?.DebugInformation
/// StreamResponse and kept internal. If we later make this public, we might need to refine this.
/// </remarks>
[JsonIgnore]
internal IEnumerable<IDisposable>? LinkedDisposables { get; set; }
protected internal IEnumerable<IDisposable>? LinkedDisposables { get; internal set; }

/// <summary>
/// Allows the response to identify that the response stream should NOT be automatically disposed.
Expand All @@ -55,6 +55,6 @@ public override string ToString() => ApiCallDetails?.DebugInformation
/// Currently only used by StreamResponse and therefore internal.
/// </remarks>
[JsonIgnore]
internal virtual bool LeaveOpen { get; } = false;
protected internal virtual bool LeaveOpen { get; } = false;
}

Loading