Skip to content

[Backport 7.13] Ensure we dispose of the response stream in all cases #5787

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 5, 2021
Merged

[Backport 7.13] Ensure we dispose of the response stream in all cases #5787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 11 additions & 6 deletions src/Elasticsearch.Net/Connection/HttpWebRequestConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
namespace Elasticsearch.Net
{
#if DOTNETCORE
[Obsolete("CoreFX HttpWebRequest uses HttpClient under the covers but does not reuse HttpClient instances, do NOT use on .NET core only used as the default on Full Framework")]
[Obsolete(
"CoreFX HttpWebRequest uses HttpClient under the covers but does not reuse HttpClient instances, do NOT use on .NET core only used as the default on Full Framework")]
#endif
public class HttpWebRequestConnection : IConnection
{
Expand Down Expand Up @@ -52,8 +53,10 @@ public virtual TResponse Request<TResponse>(RequestData requestData)
using (var stream = request.GetRequestStream())
{
if (requestData.HttpCompression)
{
using (var zipStream = new GZipStream(stream, CompressionMode.Compress))
data.Write(zipStream, requestData.ConnectionSettings);
}
else
data.Write(stream, requestData.ConnectionSettings);
}
Expand All @@ -68,7 +71,7 @@ public virtual TResponse Request<TResponse>(RequestData requestData)

//http://msdn.microsoft.com/en-us/library/system.net.httpwebresponse.getresponsestream.aspx
//Either the stream or the response object needs to be closed but not both although it won't
//throw any errors if both are closed atleast one of them has to be Closed.
//throw any errors if both are closed at least one of them has to be Closed.
//Since we expose the stream we let closing the stream determining when to close the connection
var httpWebResponse = (HttpWebResponse)request.GetResponse();
HandleResponse(httpWebResponse, out statusCode, out responseStream, out mimeType);
Expand Down Expand Up @@ -125,8 +128,10 @@ CancellationToken cancellationToken
using (var stream = await apmGetRequestStreamTask.ConfigureAwait(false))
{
if (requestData.HttpCompression)
{
using (var zipStream = new GZipStream(stream, CompressionMode.Compress))
await data.WriteAsync(zipStream, requestData.ConnectionSettings, cancellationToken).ConfigureAwait(false);
}
else
await data.WriteAsync(stream, requestData.ConnectionSettings, cancellationToken).ConfigureAwait(false);
}
Expand Down Expand Up @@ -165,8 +170,7 @@ CancellationToken cancellationToken
}
responseStream ??= Stream.Null;
var response = await ResponseBuilder.ToResponseAsync<TResponse>
(requestData, ex, statusCode, warnings, responseStream, mimeType, cancellationToken)
.ConfigureAwait(false);
(requestData, ex, statusCode, warnings, responseStream, mimeType, cancellationToken).ConfigureAwait(false);

// set TCP and threadpool stats on the response here so that in the event the request fails after the point of
// gathering stats, they are still exposed on the call details. Ideally these would be set inside ResponseBuilder.ToResponse,
Expand Down Expand Up @@ -205,7 +209,7 @@ protected virtual void SetServerCertificateValidationCallBackIfNeeded(HttpWebReq
#else
if (callback != null)
throw new Exception("Mono misses ServerCertificateValidationCallback on HttpWebRequest");
#endif
#endif
}

protected virtual HttpWebRequest CreateWebRequest(RequestData requestData)
Expand Down Expand Up @@ -311,8 +315,10 @@ protected virtual void SetBasicAuthenticationIfNeeded(HttpWebRequest request, Re
if (!string.IsNullOrEmpty(requestData.Uri.UserInfo))
userInfo = Uri.UnescapeDataString(requestData.Uri.UserInfo);
else if (requestData.BasicAuthorizationCredentials != null)
{
userInfo =
$"{requestData.BasicAuthorizationCredentials.Username}:{requestData.BasicAuthorizationCredentials.Password.CreateString()}";
}

if (string.IsNullOrWhiteSpace(userInfo))
return;
Expand All @@ -336,7 +342,6 @@ protected virtual bool SetApiKeyAuthenticationIfNeeded(HttpWebRequest request, R

request.Headers["Authorization"] = $"ApiKey {apiKey}";
return true;

}

/// <summary>
Expand Down
10 changes: 9 additions & 1 deletion src/Elasticsearch.Net/Transport/Pipeline/ResponseBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public static TResponse ToResponse<TResponse>(
// Only attempt to set the body if the response may have content
if (MayHaveBody(statusCode, requestData.Method))
response = SetBody<TResponse>(details, requestData, responseStream, mimeType);
else
responseStream.Dispose();

response ??= new TResponse();

Expand All @@ -67,6 +69,8 @@ public static async Task<TResponse> ToResponseAsync<TResponse>(
// Only attempt to set the body if the response may have content
if (MayHaveBody(statusCode, requestData.Method))
response = await SetBodyAsync<TResponse>(details, requestData, responseStream, mimeType, cancellationToken).ConfigureAwait(false);
else
responseStream.Dispose();

response ??= new TResponse();

Expand All @@ -78,7 +82,7 @@ public static async Task<TResponse> ToResponseAsync<TResponse>(
/// A helper which returns true if the response could potentially have a body.
/// </summary>
private static bool MayHaveBody(int? statusCode, HttpMethod httpMethod) =>
!statusCode.HasValue || (statusCode.Value != 204 && httpMethod != HttpMethod.HEAD);
!statusCode.HasValue || statusCode.Value != 204 && httpMethod != HttpMethod.HEAD;

private static ApiCallDetails Initialize(
RequestData requestData, Exception exception, int? statusCode, IEnumerable<string> warnings, string mimeType
Expand All @@ -91,8 +95,10 @@ private static ApiCallDetails Initialize(
if (allowedStatusCodes.Contains(-1) || allowedStatusCodes.Contains(statusCode.Value))
success = true;
else
{
success = requestData.ConnectionSettings
.StatusCodeToResponseSuccess(requestData.Method, statusCode.Value);
}
}

// We don't validate the content-type (MIME type) for HEAD requests or responses that have no content (204 status code).
Expand Down Expand Up @@ -170,8 +176,10 @@ private static async Task<TResponse> SetBodyAsync<TResponse>(

var serializer = requestData.ConnectionSettings.RequestResponseSerializer;
if (requestData.CustomResponseBuilder != null)
{
return await requestData.CustomResponseBuilder.DeserializeResponseAsync(serializer, details, responseStream, cancellationToken)
.ConfigureAwait(false) as TResponse;
}

return !RequestData.ValidResponseContentType(requestData.Accept, mimeType)
? null
Expand Down