Default MaximumRetries with not proper handling of httpwebrequest.getresponse can lead to thread hang

[avandych]

NEST/Elasticsearch.Net version: 2.0.5

Elasticsearch version: 2.4.1

Description of the problem including expected versus actual behavior:
In our environment we experienced strange issue with our machines. They just started to burn all CPU resources and hang.
After investigation we found the place where all our threads hang

After further investigation we also found that maybe HandleException method in HttpConnection doesn't properly handles situation when we got some error in request.GetResponse() and response.GetResponseStream() in HandleException method returns null (looks like underlining tcp connection isn't disposed).

In ASP.NET application ServicePointManager.DefaultConnectionLimit is set to 10.
In your client MaxRetries property in RequestPipeline defaults to int.MaxValue when not configured by user.

What we think happens is - Elasticsearch client uses all 10 connections and don't disposes them properly, connection number 11 established by client hangs in queue waiting when ServicePointManager allows it to be used, CPU is burst to full core usage on that thread.

After we set ConnectionSettings MaximumRetries to 3 all works fine, without cpu burst and hanging.

Our problem was that we used default settings for client.
Many developers that start with your client, start from default settings, and I suggest that it will be good to set default MaximumRetries to 3, so they will not get into similar situation and spend some unnecessary amount of time to figure out what is going on. (3 because ServicePointManager.DefaultConnectionLimit in console app defaults to 3).

The above is only an assumption but I think that it's worth to check it.

[Mpdreamz]

Thank you @SPANIOL for such a detailed bug report, investigating this now and trying to capture this in a test.

[Mpdreamz]

Confirmed we do not handle the case where response.GetResponseStream() returns null in our WebException handling.

As per: http://msdn.microsoft.com/en-us/library/system.net.httpwebresponse.getresponsestream.aspx

You must call either the Stream.Close or the HttpWebResponse.Close method to close the stream and release the connection for reuse. It is not necessary to call both Stream.Close and HttpWebResponse.Close, but doing so does not cause an error. Failure to close the stream will cause your application to run out of connections.

We rely on disposing the stream only but in this case we need to forcefully call dispose on response instead.

While running through this spotted another theoretical bug. There is side effect problem when ResponseBuilder has no StatusCode set on it but does have a Stream associated with it. However this should never occur since StatusCode is always set before Stream in HttpConnection. Added tests for this and made sure to make sure this won't introduce a similar problem in the future
Sidenote: If DisableDirectStreaming is set this problem does not exist because SwapStreams always disposes.

[Mpdreamz]

Some comments on DefaultConnectionLimit, HttpConnection forcefully sets it to to 10k for the current servicepoint. If you inspect using wireshark or similar you'll see it will never actually get to 10k, I typically see it peak at ~70 after growing slowly, and the tcp sockets are being reused.

The MaxRetries is always bound by the number of nodes in your connectionpool if not explicitly stated.

[Mpdreamz]

Digging through the reference source it appears not impossible for GetResponseStream() to return null but also unlikely: https://referencesource.microsoft.com/#System/net/System/Net/HttpWebResponse.cs,405

Can you share more about the type of response/exception you are seeing in these cases?

[avandych]

First of all, thank you, @Mpdreamz , for fast response.

Actually it's first time that i looked into sources of the client, so i know little about how it works in complex. But i cannot find a place in HttpConnection where DefaultConnectionLimit is forcefully set to 10k. About MaxRetries in Request pipeline, it's my mistake and now i see that with StickyConnectionPool that we use it defaults to this.InternalNodes.Count - 1;. We have only 1 node, so i think that for us MaxRetries is 0. Now question - how many times it will actually perform retry with zero in MaxRetries? In BadResponse method of RequestPipeline i can see if (this.Retried >= this.MaxRetries && this.MaxRetries > 0).

[Mpdreamz]

@SPANIOL we set the connectionlimit here: https://github.com/elastic/elasticsearch-net/blob/master/src/Elasticsearch.Net/Connection/HttpConnection.cs#L78

requestServicePoint.ConnectionLimit defaults to the static ServicePointManager.DefaultConnectionLimit

With 1 node it will not retry by default, the number of nodes in the connectionpool is also the upper cap.
Our cluster failover is build to fail fast not to force success, subtle but important difference 😄

We have some docs on this subject here which are generated from source here

if anything is missing or is unclear let me know as these are a work in progress 👍