Skip to content

Commit 7e504eb

Browse files
MpdreamzMpdreamz
committed
when not using connection pooling and an exception occurs in the IConnection layer do not retry and bubble out immediatly, the chances of the call succeeding a second time are slim
1 parent 0b92314 commit 7e504eb

File tree

5 files changed

+133
-22
lines changed

5 files changed

+133
-22
lines changed

Diff for: ‎src/Elasticsearch.Net/Connection/Transport.cs

+5-5
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ private ElasticsearchResponse<T> DoRequest<T>(TransportRequestState<T> requestSt
369369
catch (Exception e)
370370
{
371371
requestState.SeenExceptions.Add(e);
372-
if (maxRetries == 0 && retried == 0)
372+
if (!requestState.UsingPooling || maxRetries == 0 && retried == 0)
373373
{
374374
if (_throwMaxRetry)
375375
new MaxRetryException(e);
@@ -489,8 +489,8 @@ private Task<ElasticsearchResponse<T>> FinishOrRetryRequestAsync<T>(TransportReq
489489
if (t.IsFaulted)
490490
{
491491
rq.Dispose();
492-
if (maxRetries == 0 && retried == 0) throw t.Exception;
493492
requestState.SeenExceptions.Add(t.Exception);
493+
if (!requestState.UsingPooling || maxRetries == 0 && retried == 0) throw t.Exception;
494494
return this.RetryRequestAsync<T>(requestState);
495495
}
496496

@@ -633,13 +633,13 @@ private ElasticsearchServerError ThrowOrGetErrorFromStreamResponse<T>(
633633
TransportRequestState<T> requestState,
634634
ElasticsearchResponse<Stream> streamResponse)
635635
{
636-
if (IsValidResponse(requestState, streamResponse))
637-
return null;
638-
639636
if (((streamResponse.HttpStatusCode.HasValue && streamResponse.HttpStatusCode.Value <= 0)
640637
|| !streamResponse.HttpStatusCode.HasValue) && streamResponse.OriginalException != null)
641638
throw streamResponse.OriginalException;
642639

640+
if (IsValidResponse(requestState, streamResponse))
641+
return null;
642+
643643
ElasticsearchServerError error = null;
644644

645645
var type = typeof(T);

Diff for: ‎src/Elasticsearch.Net/Domain/Response/ElasticsearchResponse.cs

+13-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.CodeDom;
33
using System.Collections.Generic;
4+
using System.ComponentModel;
45
using System.Configuration;
56
using System.Globalization;
67
using System.Linq;
@@ -12,6 +13,7 @@
1213
using System.Threading.Tasks;
1314
using Elasticsearch.Net;
1415
using Elasticsearch.Net.Connection;
16+
using Elasticsearch.Net.ConnectionPool;
1517
using Elasticsearch.Net.Exceptions;
1618
using Elasticsearch.Net.Serialization;
1719

@@ -125,8 +127,12 @@ public bool SuccessOrKnownError
125127
{
126128
get
127129
{
130+
var pool = this.Settings.ConnectionPool;
131+
var usingPool = pool != null && pool.GetType() != typeof(SingleNodeConnectionPool);
132+
128133
return this.Success ||
129-
(this.HttpStatusCode.HasValue
134+
(!usingPool && this.HttpStatusCode.GetValueOrDefault(1) < 0)
135+
|| (this.HttpStatusCode.HasValue
130136
&& this.HttpStatusCode.Value != 503 //service unavailable needs to be retried
131137
&& this.HttpStatusCode.Value != 502 //bad gateway needs to be retried
132138
&& ((this.HttpStatusCode.Value >= 400 && this.HttpStatusCode.Value < 599)));
@@ -162,22 +168,26 @@ public static ElasticsearchResponse<T> CreateError(IConnectionConfigurationValue
162168
return cs;
163169
}
164170

165-
public static ElasticsearchResponse<T> Create(IConnectionConfigurationValues settings, int statusCode, string method, string path, byte[] request)
171+
public static ElasticsearchResponse<T> Create(
172+
IConnectionConfigurationValues settings, int statusCode, string method, string path, byte[] request, Exception innerException = null)
166173
{
167174
var cs = new ElasticsearchResponse<T>(settings, statusCode);
168175
cs.Request = request;
169176
cs.RequestUrl = path;
170177
cs.RequestMethod = method;
178+
cs.OriginalException = innerException;
171179
return cs;
172180
}
173181

174-
public static ElasticsearchResponse<T> Create(IConnectionConfigurationValues settings, int statusCode, string method, string path, byte[] request, T response)
182+
public static ElasticsearchResponse<T> Create(
183+
IConnectionConfigurationValues settings, int statusCode, string method, string path, byte[] request, T response, Exception innerException = null)
175184
{
176185
var cs = new ElasticsearchResponse<T>(settings, statusCode);
177186
cs.Request = request;
178187
cs.RequestUrl = path;
179188
cs.RequestMethod = method;
180189
cs.Response = response;
190+
cs.OriginalException = innerException;
181191
return cs;
182192
}
183193

Diff for: ‎src/Tests/Elasticsearch.Net.Tests.Unit/Connection/RetryTests.cs

+15-13
Original file line numberDiff line numberDiff line change
@@ -22,28 +22,28 @@ public class RetryTests
2222
.MaximumRetries(_retries);
2323

2424
[Test]
25-
public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes()
25+
public void OnConnectionException_WithoutPooling_DoNotRetry()
2626
{
2727
using (var fake = new AutoFake(callsDoNothing: true))
2828
{
2929
fake.Provide<IConnectionConfigurationValues>(_connectionConfig);
3030
FakeCalls.ProvideDefaultTransport(fake);
3131

3232
var getCall = FakeCalls.GetSyncCall(fake);
33-
getCall.Throws<Exception>();
33+
getCall.Throws((o)=> new Exception("inner"));
3434

3535
var client = fake.Resolve<ElasticsearchClient>();
3636

3737
client.Settings.MaxRetries.Should().Be(_retries);
3838

39-
Assert.Throws<MaxRetryException>(() => client.Info());
40-
getCall.MustHaveHappened(Repeated.Exactly.Times(_retries + 1));
41-
39+
var e = Assert.Throws<Exception>(() => client.Info());
40+
e.Message.Should().Be("inner");
41+
getCall.MustHaveHappened(Repeated.Exactly.Once);
4242
}
4343
}
4444

4545
[Test]
46-
public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes_HardIConnectionException_Async()
46+
public void Hard_IConnectionException_AsyncCall_WithoutPooling_DoesNot_Retry_AndRethrows()
4747
{
4848
using (var fake = new AutoFake(callsDoNothing: true))
4949
{
@@ -52,19 +52,20 @@ public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes_HardIConnectionE
5252
var getCall = FakeCalls.GetCall(fake);
5353

5454
//return a started task that throws
55-
getCall.Throws<Exception>();
55+
getCall.Throws((o)=> new Exception("inner"));
5656

5757
var client = fake.Resolve<ElasticsearchClient>();
5858

5959
client.Settings.MaxRetries.Should().Be(_retries);
6060

61-
Assert.Throws<MaxRetryException>(async () => await client.InfoAsync());
62-
getCall.MustHaveHappened(Repeated.Exactly.Times(_retries + 1));
61+
var e = Assert.Throws<Exception>(async () => await client.InfoAsync());
62+
e.Message.Should().Be("inner");
63+
getCall.MustHaveHappened(Repeated.Exactly.Once);
6364
}
6465
}
6566

6667
[Test]
67-
public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes_Async()
68+
public void Soft_IConnectionException_AsyncCall_WithoutPooling_DoesNot_Retry_AndRethrows()
6869
{
6970
using (var fake = new AutoFake(callsDoNothing: true))
7071
{
@@ -73,7 +74,7 @@ public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes_Async()
7374
var getCall = FakeCalls.GetCall(fake);
7475

7576
//return a started task that throws
76-
Func<ElasticsearchResponse<Stream>> badTask = () => { throw new Exception(); };
77+
Func<ElasticsearchResponse<Stream>> badTask = () => { throw new Exception("inner"); };
7778
var t = new Task<ElasticsearchResponse<Stream>>(badTask);
7879
t.Start();
7980
getCall.Returns(t);
@@ -82,8 +83,9 @@ public void ThrowsMaxRetryException_AndRetriesTheSpecifiedTimes_Async()
8283

8384
client.Settings.MaxRetries.Should().Be(_retries);
8485

85-
Assert.Throws<MaxRetryException>(async () => await client.InfoAsync());
86-
getCall.MustHaveHappened(Repeated.Exactly.Times(_retries + 1));
86+
var e = Assert.Throws<Exception>(async () => await client.InfoAsync());
87+
e.Message.Should().Be("inner");
88+
getCall.MustHaveHappened(Repeated.Exactly.Once);
8789
}
8890
}
8991

Diff for: ‎src/Tests/Elasticsearch.Net.Tests.Unit/Connection/SingleNodeConnectionPoolTests.cs

+88
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.IO;
4+
using System.Linq;
5+
using System.Net;
6+
using System.Net.WebSockets;
7+
using System.Runtime.InteropServices;
8+
using System.Threading.Tasks;
9+
using Autofac;
10+
using Autofac.Extras.FakeItEasy;
11+
using Elasticsearch.Net.Connection;
12+
using Elasticsearch.Net.Connection.Configuration;
13+
using Elasticsearch.Net.ConnectionPool;
14+
using Elasticsearch.Net.Exceptions;
15+
using Elasticsearch.Net.Providers;
16+
using Elasticsearch.Net.Tests.Unit.Stubs;
17+
using FakeItEasy;
18+
using FluentAssertions;
19+
using NUnit.Framework;
20+
21+
namespace Elasticsearch.Net.Tests.Unit.Connection
22+
{
23+
[TestFixture]
24+
public class SingleNodeConnectionPoolTests
25+
{
26+
private readonly ConnectionConfiguration _config;
27+
private ElasticsearchResponse<Stream> _ok;
28+
private ElasticsearchResponse<Stream> _bad;
29+
30+
31+
public SingleNodeConnectionPoolTests()
32+
{
33+
_config = new ConnectionConfiguration(new Uri("http://localhost:9200"))
34+
.MaximumRetries(2);
35+
36+
_ok = FakeResponse.Ok(_config);
37+
_bad = FakeResponse.Any(_config, -1);
38+
39+
40+
}
41+
42+
private ITransport ProvideTransport(AutoFake fake)
43+
{
44+
var param = new TypedParameter(typeof(IDateTimeProvider), null);
45+
return fake.Provide<ITransport, Transport>(param);
46+
}
47+
48+
[Test]
49+
public void Should_Not_Retry_On_IConnection_Exception()
50+
{
51+
using (var fake = new AutoFake(callsDoNothing: true))
52+
{
53+
//set up connection configuration that holds a connection pool
54+
//with '_uris' (see the constructor)
55+
fake.Provide<IConnectionConfigurationValues>(_config);
56+
//prove a real HttpTransport with its unspecified dependencies
57+
//as fakes
58+
this.ProvideTransport(fake);
59+
60+
//set up fake for a call on IConnection.GetSync so that it always throws
61+
//an exception
62+
var getCall = FakeCalls.GetSyncCall(fake);
63+
getCall.Returns(FakeResponse.AnyWithException(_config, -1, innerException: new Exception("inner")));
64+
var pingCall = FakeCalls.PingAtConnectionLevel(fake);
65+
pingCall.Returns(_ok);
66+
67+
//create a real ElasticsearchClient with it unspecified dependencies
68+
//as fakes
69+
var client = fake.Resolve<ElasticsearchClient>();
70+
71+
//our settings dictate retrying 2 times
72+
client.Settings.MaxRetries.Should().Be(2);
73+
74+
//the single node connection pool always reports 0 for maxretries
75+
client.Settings.ConnectionPool.MaxRetries.Should().Be(0);
76+
77+
//
78+
var exception = Assert.Throws<Exception>(()=> client.Info());
79+
exception.Message.Should().Be("inner");
80+
//the GetSync method must in total have been called the number of nodes times.
81+
getCall.MustHaveHappened(Repeated.Exactly.Once);
82+
83+
}
84+
}
85+
86+
87+
}
88+
}

Diff for: ‎src/Tests/Elasticsearch.Net.Tests.Unit/Stubs/FakeResponse.cs

+12-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1+
using System;
12
using System.Collections.Generic;
23
using System.IO;
34
using System.Text;
45
using System.Threading.Tasks;
56
using Elasticsearch.Net.Connection;
7+
using FakeItEasy.ExtensionSyntax.Full;
68

79
namespace Elasticsearch.Net.Tests.Unit.Stubs
810
{
@@ -54,7 +56,16 @@ public static ElasticsearchResponse<Stream> Any(
5456
{
5557
return ElasticsearchResponse<Stream>.Create(config, statusCode, method, path, null, response);
5658
}
57-
59+
public static ElasticsearchResponse<Stream> AnyWithException(
60+
IConnectionConfigurationValues config,
61+
int statusCode,
62+
string method = "GET",
63+
string path = "/",
64+
Stream response = null,
65+
Exception innerException = null)
66+
{
67+
return ElasticsearchResponse<Stream>.Create(config, statusCode, method, path, null, response, innerException);
68+
}
5869
public static Task<ElasticsearchResponse<Stream>> AnyAsync(
5970
IConnectionConfigurationValues config,
6071
int statusCode,

0 commit comments

Comments
 (0)