Fix sending GET and HEAD messages #3101

BastiaanLouis · 2018-02-19T22:32:27Z

When sending a HEAD or GET message, the message does not contain any content. The used HttpClient class accepts no content at all using these type of verbs. The current implementation that creates an empty BytesArrayContent actually creates a message that will not be send by the HttpClient. A ProtocolViolationException will be thrown (see https://github.com/Microsoft/referencesource/blob/master/System/net/System/Net/HttpWebRequest.cs, private void CheckProtocol(bool onRequestStream)).
This small fixes leaves the Content of the requestMessage null, only for HEAD and GET messages for which no postdata is offered.

Unfortunately there are no tests available that I can update to validate the change. To verify the bug and it's fix, I've been using the StaticConnectionPool (actually through Serilog).

…of a HEAD or GET request. HttpClient does not accept a content at all when the request is a HEAD or a GET. Even an empty ByteArray is not allowed. A ProtocolViolationException will be thrown. The 'Ping' functionality can not be activated without this fix because that is based on a HEAD message.

russcam · 2018-02-19T22:46:30Z

Could you provide some more details for this change @BastiaanLouis ?

What framework version are you using?
What OS are you running on?
What is the underlying HttpClientHandler used?
Are you running through a proxy?

Have you experienced exceptions thrown? I ask because we run integration tests on both Windows and Linux (see the Windows CI and Linux CI build links) and have not seen ProtocolViolationException thrown.

Mpdreamz · 2018-02-20T10:14:39Z

What versions of Elasticsearch and NEST or Elasticsearch.NET are you using @BastiaanLouis

BastiaanLouis · 2018-02-20T11:27:28Z

Some additional info.

I'm running on a windows 2012 server, on .NET framework 4.7
The elasticssearch server is never reached because the issue is that the request is never send out.

The following package version are used:

Serilog: 2.6.0
Serilog.Sinks.Elasticsearch: 6.1.0
Elasticsearch.NET: 6.0.0

Below a simple representation of the code that is executed by Elasticsearch.NET.
This is created by debugging through the code. The only dependency is from System.Net.Http.
Change the boolean 'faultSituation' to validate the behavior of the HttpClient.
Can someone validate whether this is the correct simulation of Elasticsearch.NET?

class Program
{
	static bool faultSituation = true;
	static void Main(string[] args)
	{
		// Copied from .\elasticsearch-net\src\elasticsearch.net\connection\httpconnection-corefx.cs
		// protected virtual HttpClientHandler CreateHttpClientHandler(RequestData requestData)
		var handler = new HttpClientHandler
		{
			AutomaticDecompression = System.Net.DecompressionMethods.None,
		};
		handler.MaxConnectionsPerServer = 80;

		// Copied from .\elasticsearch-net\src\elasticsearch.net\connection\httpconnection-corefx.cs
		// private HttpClient GetClient(RequestData requestData)
		var httpClient = new HttpClient(handler, false)
		{
			Timeout = TimeSpan.FromSeconds(5),
		};
		httpClient.DefaultRequestHeaders.ExpectContinue = false;

		// Copied from .\elasticsearch-net\src\elasticsearch.net\connection\httpconnection-corefx.cs
		// protected virtual HttpRequestMessage CreateRequestMessage(RequestData requestData)
		var requestMessage = new HttpRequestMessage(HttpMethod.Head, "http://localhost:9200");
		requestMessage.Headers.Connection.Clear();
		requestMessage.Headers.ConnectionClose = false;
		requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

		// Change to see what happens when the content is null or an empty ByteArray
		if (faultSituation)
		{
			requestMessage.Content = new ByteArrayContent(new byte[0]);
			requestMessage.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
		}
		try
		{
			httpClient.SendAsync(requestMessage).GetAwaiter().GetResult();
			Console.WriteLine($"Request succeeded");
		}
		catch (ProtocolViolationException e)
		{
			Console.WriteLine($"Protocol violation exception occured: {e}");
		}
		catch (Exception e)
		{
			Console.WriteLine($"Unknown exception occured: {e}");
		}
		Console.ReadLine();
	}
}

…tails use it in a safe wat

…is/elasticsearch-net into fix/contentbody-verb

BastiaanLouis · 2018-02-21T16:48:34Z

@russcam @Mpdreamz
I've created a more simple example of the exception. It does depends on the Elasticsearch.NET library
The problem with this exception is that it was hidden. I've added an extra commit to fix that issue.

When you execute the following code (the value of the uri doesn't matter) within a NET 4.7 console application, without the proposed commits, the resulting responsePing contained an error that does not contain the original exception (for which this PR is opened in the first place). Fixing the handling of a bad response in the RequestPipeline, makes sure the internal exception (ProtocolViolationException) will be visible in the responsePing object. The fix in HttpConnection that I've already made, will make sure the ProtocalViolationException won't occur anymore.

var uris = new Uri[] { new Uri("http://localelastic:9200") };
var connectionPool = new StaticConnectionPool(uris);
var connectionConfiguration = new ConnectionConfiguration(connectionPool);
var elasticClient = new ElasticLowLevelClient(connectionConfiguration);
var responsePing = elasticClient.Ping<DynamicResponse>(null);

Of course when executing including the fixes, the uri does matter.

russcam · 2018-02-27T00:38:27Z

I saw this issue with integration tests against 6.1.2

--------
Individual cluster running times:
- ReadOnly: 00:00:14.3842265
- UNKNOWN: 00:00:00.0002433
--------
--------
build seed:98651 integrate 6.1.2
--------
=== TEST EXECUTION SUMMARY ===
   Tests  Total: 3, Errors: 0, Failed: 0, Skipped: 0, Time: 14.393s
Building for framework net46...
  Elasticsearch.Net -> C:\Users\russ\source\elasticsearch-net-master\src\Elasticsearch.Net\bin\Debug\netstandard1.3\Elasticsearch.Net.dll
  Nest -> C:\Users\russ\source\elasticsearch-net-master\src\Nest\bin\Debug\netstandard1.3\Nest.dll
  Nest.JsonNetSerializer -> C:\Users\russ\source\elasticsearch-net-master\src\Serializers\Nest.JsonNetSerializer\bin\Debug\netstandard1.3\Nest.JsonNetSerializer.dll
  Elasticsearch.Net.Connections.HttpWebRequestConnection -> C:\Users\russ\source\elasticsearch-net-master\src\Connections\Elasticsearch.Net.Connections.HttpWebRequestConnection\bin\Debug\net46\Elasticsearch.Net.Connections.HttpWebRequestConnection.dll
C:\Program Files\dotnet\sdk\2.0.3\Microsoft.Common.CurrentVersion.targets(1988,5): warning MSB3277: Found conflicts between different versions of the same dependent assembly that could not be resolved.  These reference conflicts are listed in the build log when log verbosity is set to detailed. [C:\Users\russ\source\elasticsearch-net-master\src\Tests\Tests.csproj]
  Tests -> C:\Users\russ\source\elasticsearch-net-master\src\Tests\bin\Debug\net46\Tests.exe
Running desktop CLR tests for framework net46...
xUnit.net Console Runner (64-bit Desktop .NET 4.0.30319.42000)
--------------------
Starting tests using config:
 - TestAgainstAlreadyRunningElasticsearch: False
 - ElasticsearchVersion: 6.1.2
 - ForceReseed: True
 - Mode: Integration
 - Seed: 50919
 - ClusterFilter: ReadOnly
 - TestFilter: SuggestUsage
 - RunIntegrationTests: True
 - RunUnitTests: False
 - Random:
        - SourceSerializer: True
        - TypedKeys: True
        - OldConnection: False
--------------------
  Discovering: Tests
  Discovered:  Tests
  Starting:    Tests
Starting: C:\Users\russ\AppData\Roaming\NEST\6.1.2\elasticsearch-6.1.2\bin\elasticsearch.bat -E cluster.name=readonly-cluster-ac7b8b -E node.name=readonly-node-ac7b8b -E path.repo=C:\Users\russ\AppData\Roaming\NEST\6.1.2\repositories -E path.data=C:\Users\russ\AppData\Roaming\NEST\6.1.2\elasticsearch-6.1.2\data\readonly-cluster-ac7b8b -E http.port=9200 -E node.attr.testingcluster=true -E node.attr.gateway=true -E xpack.security.enabled=false -E xpack.security.http.ssl.enabled=false -E xpack.security.authc.realms.pki1.enabled=false -E search.remote.connect=true -E script.max_compilations_rate=10000/1m -E script.allowed_types=inline,stored
Killing elasticsearch PID 14016
Stopping... node has started and ran integrations: True
Node started on port: 9200 using PID: 14016
Elasticsearch.Net.UnexpectedElasticsearchClientException: Cannot send a content-body with this verb-type. ---> System.Net.ProtocolViolationException: Cannot send a content-body with this verb-type.
   at System.Net.HttpWebRequest.CheckProtocol(Boolean onRequestStream)
   at System.Net.HttpWebRequest.BeginGetRequestStream(AsyncCallback callback, Object state)
   at System.Net.Http.HttpClientHandler.StartGettingRequestStream(RequestState state)
   at System.Net.Http.HttpClientHandler.PrepareAndStartContentUpload(RequestState state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Net.Http.HttpClient.<FinishSendAsync>d__58.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Elasticsearch.Net.HttpConnection.Request[TResponse](RequestData requestData)
   at Elasticsearch.Net.RequestPipeline.CallElasticsearch[TResponse](RequestData requestData)
   at Elasticsearch.Net.Transport`1.Request[TResponse](HttpMethod method, String path, PostData data, IRequestParameters requestParameters)
   --- End of inner exception stack trace ---
   at Elasticsearch.Net.Transport`1.Request[TResponse](HttpMethod method, String path, PostData data, IRequestParameters requestParameters)
   at Elasticsearch.Net.ElasticLowLevelClient.DoRequest[TResponse](HttpMethod method, String path, PostData data, IRequestParameters requestParameters)
   at Elasticsearch.Net.ElasticLowLevelClient.CatPlugins[TResponse](CatPluginsRequestParameters requestParameters)
   at Nest.LowLevelDispatch.CatPluginsDispatch[TResponse](IRequest`1 p)
   at Nest.ElasticClient.<>c__DisplayClass82_0`3.<DoCat>b__1(TRequest p, SerializableData`1 d)
   at Nest.ElasticClient.Nest.IHighLevelToLowLevelDispatcher.Dispatch[TRequest,TQueryString,TResponse](TRequest request, Func`3 responseGenerator, Func`3 dispatch)
   at Nest.ElasticClient.DoCat[TRequest,TParams,TCatRecord](TRequest request, Func`2 dispatch)
   at Nest.ElasticClient.CatPlugins(ICatPluginsRequest request)
   at Nest.ElasticClient.CatPlugins(Func`2 selector)
   at Tests.Framework.ManagedElasticsearch.Tasks.ValidationTasks.ValidatePluginsTask.Validate(IElasticClient client, NodeConfiguration configuration) in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\ManagedElasticsearch\Tasks\ValidationTasks\ValidatePluginsTask.cs:line 21
   at Tests.Framework.ManagedElasticsearch.Tasks.NodeTaskRunner.<>c__DisplayClass20_0.<ValidateAfterStart>b__0(NodeValidationTaskBase t, NodeConfiguration n, NodeFileSystem fs) in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\ManagedElasticsearch\Tasks\NodeTaskRunner.cs:line 70
   at Tests.Framework.ManagedElasticsearch.Tasks.NodeTaskRunner.Iterate[T](IEnumerable`1 collection, Action`3 act, Boolean log) in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\ManagedElasticsearch\Tasks\NodeTaskRunner.cs:line 93
   at Tests.Framework.ManagedElasticsearch.Tasks.NodeTaskRunner.ValidateAfterStart(IElasticClient client) in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\ManagedElasticsearch\Tasks\NodeTaskRunner.cs:line 70
   at Tests.Framework.ManagedElasticsearch.Clusters.ClusterBase.Start() in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\ManagedElasticsearch\Clusters\ClusterBase.cs:line 48
   at Xunit.TestAssemblyRunner.<IntegrationPipeline>d__17.MoveNext() in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\Xunit\TestAssemblyRunner.cs:line 111
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Xunit.TestAssemblyRunner.<RunTestCollectionsAsync>d__15.MoveNext() in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\Xunit\TestAssemblyRunner.cs:line 69
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Xunit.Sdk.TestAssemblyRunner`1.<RunAsync>d__41.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Xunit.TestFrameworkExecutor.<RunTestCases>d__1.MoveNext() in C:\Users\russ\source\elasticsearch-net-master\src\Tests\Framework\Xunit\TestFrameworkExecutor.cs:line 26
  Finished:    Tests
System.Runtime.Serialization.SerializationException: Type 'Elasticsearch.Net.UnexpectedElasticsearchClientException' in Assembly 'Elasticsearch.Net, Version=0.0.0.0, Culture=neutral, PublicKeyToken=96c599bbe3e70f5d' is not marked as serializable.
   at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.InitSerialize(Object obj, ISurrogateSelector surrogateSelector, StreamingContext context, SerObjectInfoInit serObjectInfoInit, IFormatterConverter converter, ObjectWriter objectWriter, SerializationBinder binder)
   at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.Serialize(Object obj, ISurrogateSelector surrogateSelector, StreamingContext context, SerObjectInfoInit serObjectInfoInit, IFormatterConverter converter, ObjectWriter objectWriter, SerializationBinder binder)
   at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.Serialize(Object graph, Header[] inHeaders, __BinaryWriter serWriter, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializationStream, Object graph, Header[] headers, Boolean fCheck)
   at System.Runtime.Remoting.Channels.CrossAppDomainSerializer.SerializeObject(Object obj, MemoryStream stm)
   at System.AppDomain.Serialize(Object o)
   at System.AppDomain.MarshalObject(Object o)

Finished Target: Integrate

---------------------------------------------------------------------
Build Time Report
---------------------------------------------------------------------
Target      Duration
------      --------
Start       00:00:00.0031179
Restore     00:00:05.6266851
Integrate   00:00:37.9717888
Total:      00:00:43.6400037
---------------------------------------------------------------------
Status:     Ok
---------------------------------------------------------------------

russcam · 2018-02-27T00:43:08Z

I think we may have never seen this before because net46 version always used the older HttpWebRequest based HttpConnection, but master only builds netstandard1.3, which uses the HttpClient based HttpConnection on all target framework versions.

BastiaanLouis · 2018-03-05T09:21:55Z

How do you suggest to fix this issue @russcam ? Because we are building to .net framework 4.6.x/4.7 the net46 assembly from the package will be picked. Is the solution in the pull request the correct way of fixing it?

russcam · 2018-03-06T02:38:24Z

Apologies for not getting back to you sooner @BastiaanLouis, I've currently been travelling.

I think in the case of any request where there is not request data written to the request stream, we can forgo setting content and the Content-Type header i.e. inside CreateRequestMessage(RequestData requestData)

var data = requestData.PostData;

if (data != null)
{
	var stream = requestData.MemoryStreamFactory.Create();
	requestMessage.Content = new StreamContent(stream);
	requestMessage.Content.Headers.ContentType = new MediaTypeHeaderValue(requestData.RequestMimeType);
	if (requestData.HttpCompression)
	{
		requestMessage.Content.Headers.Add("Content-Encoding", "gzip");
		requestMessage.Headers.AcceptEncoding.Add(new StringWithQualityHeaderValue("gzip"));
		requestMessage.Headers.AcceptEncoding.Add(new StringWithQualityHeaderValue("deflate"));
		using (var zipStream = new GZipStream(stream, CompressionMode.Compress, true))
			data.Write(zipStream, requestData.ConnectionSettings);
	}
	else
	    data.Write(stream, requestData.ConnectionSettings);
	stream.Position = 0;
}

return requestMessage;

BastiaanLouis · 2018-03-07T07:58:00Z

Nice @russcam mentioned cleaning up method CreateRequestMessage(RequestData requestData). Wasn't sure why the empty body was used in the first place. This makes it more readable.

russcam · 2018-03-07T18:39:47Z

Thanks @BastiaanLouis , will take a look now.

The empty body was required in order to set a Content-Type header. In a much earlier version of Elasticsearch, the Content-Type header was incorrectly used for both the inward content type and the outward content type; the latter was corrected to use the Accept header.

BastiaanLouis · 2018-03-14T19:05:44Z

Great!
@russcam if you need any additional help, info of anything just let me know!

Mpdreamz · 2018-03-15T10:19:22Z

This looks good to me, thanks for your efforts on this one @BastiaanLouis 👍

* The Http specs says that the body can be ignored by the host in case of a HEAD or GET request. HttpClient does not accept a content at all when the request is a HEAD or a GET. Even an empty ByteArray is not allowed. A ProtocolViolationException will be thrown. The 'Ping' functionality can not be activated without this fix because that is based on a HEAD message. * callDetails can be null so just like the rest of the usages of callDetails use it in a safe wat * Simplify handling empty data

* The Http specs says that the body can be ignored by the host in case of a HEAD or GET request. HttpClient does not accept a content at all when the request is a HEAD or a GET. Even an empty ByteArray is not allowed. A ProtocolViolationException will be thrown. The 'Ping' functionality can not be activated without this fix because that is based on a HEAD message. * callDetails can be null so just like the rest of the usages of callDetails use it in a safe wat * Simplify handling empty data Conflicts: src/Elasticsearch.Net/Connection/HttpConnection-CoreFx.cs src/Elasticsearch.Net/Transport/Pipeline/RequestPipeline.cs

Mpdreamz · 2018-03-15T10:31:02Z

ported to 6.x and 5.x

BastiaanLouis · 2018-03-15T14:57:01Z

Great, thx! Looking forward to use the new package.

russcam · 2018-03-17T00:16:58Z

Thanks @Mpdreamz for picking up my slack on this. And apologies @BastiaanLouis for the delay here; this PR got caught up in my moving house and annual leave 😞

BastiaanLouis · 2018-04-26T12:16:30Z

@russcam @Mpdreamz When can I expect a new version of the ElasticSearch.NET nuget package on nuget.org with this (and probably other) fixes and features?

russcam · 2018-04-26T13:00:51Z

@BastiaanLouis We were hoping to get a new release out today, but backporting changes from master to 6.x and 5.x took longer than planned. We are aiming to get a release out tomorrow.

BastiaanLouis changed the title ~~The Http specs says that the body can be ignored by the host in case …~~ Fix sending GET and HEAD messages Feb 19, 2018

BastiaanLouis added 3 commits February 20, 2018 14:48

Merge branch 'master' into fix/contentbody-verb

79b678b

callDetails can be null so just like the rest of the usages of callDe…

63752e8

…tails use it in a safe wat

Merge branch 'fix/contentbody-verb' of https://github.com/BastiaanLou…

51d872f

…is/elasticsearch-net into fix/contentbody-verb

BastiaanLouis added 2 commits March 7, 2018 08:36

Simplify handling empty data

c33e885

Merge branch 'master' into fix/contentbody-verb

9df7e87

Merge branch 'master' into fix/contentbody-verb

5616e01

Mpdreamz merged commit 649152f into elastic:master Mar 15, 2018

BastiaanLouis deleted the fix/contentbody-verb branch March 15, 2018 14:57

karolz-ms mentioned this pull request May 18, 2018

Error handling problem Azure/diagnostics-eventflow#215

Closed

snakefoot mentioned this pull request Dec 4, 2018

Nullreference exception markmcdowell/NLog.Targets.ElasticSearch#77

Closed

Fix sending GET and HEAD messages #3101

Fix sending GET and HEAD messages #3101

Uh oh!

Conversation

BastiaanLouis commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russcam commented Feb 19, 2018

Uh oh!

Mpdreamz commented Feb 20, 2018

Uh oh!

BastiaanLouis commented Feb 20, 2018

Uh oh!

BastiaanLouis commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russcam commented Feb 27, 2018

Uh oh!

russcam commented Feb 27, 2018

Uh oh!

BastiaanLouis commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russcam commented Mar 6, 2018

Uh oh!

BastiaanLouis commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russcam commented Mar 7, 2018

Uh oh!

BastiaanLouis commented Mar 14, 2018

Uh oh!

Mpdreamz commented Mar 15, 2018

Uh oh!

Mpdreamz commented Mar 15, 2018

Uh oh!

BastiaanLouis commented Mar 15, 2018

Uh oh!

russcam commented Mar 17, 2018

Uh oh!

BastiaanLouis commented Apr 26, 2018

Uh oh!

russcam commented Apr 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BastiaanLouis commented Feb 19, 2018 •

edited

Loading

BastiaanLouis commented Feb 21, 2018 •

edited

Loading

BastiaanLouis commented Mar 5, 2018 •

edited

Loading

BastiaanLouis commented Mar 7, 2018 •

edited

Loading