From c68fdee9c594c4973385bf403ae8910e9416f05f Mon Sep 17 00:00:00 2001
From: Alexander Turenko <alexander.turenko@tarantool.org>
Date: Wed, 15 Jul 2020 02:37:21 +0300
Subject: [PATCH] Give meaningful error when retry_count is zero

Clarified docs about this setting.

There may be a confusion whether the setting means overall amount of
connection attempts or amount after the initial unsuccessful attempt.
Usually a retries amount option means the latter, but here it means the
former.

Decided to don't change behaviour or the setting name to provide perfect
backward compatibility, but give meaningful error and clarify docs. See
the discussion in [1].

[1]: https://github.com/tarantool/tarantool-php/pull/145#discussion_r241255667

Fixes #83
---
 README.md           |  2 +-
 src/tarantool.c     |  5 +++++
 test/CreateTest.php | 28 ++++++++++++++++++++++++++++
 test/shared/box.lua | 17 +++++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 2c12a88..ebf692b 100644
--- a/README.md
+++ b/README.md
@@ -67,7 +67,7 @@ Place it into project library path in your IDE.
 
 * `tarantool.persistent` - Enable persistent connections (don't close connections between sessions) (defaults: True, **can't be changed in runtime**)
 * `tarantool.timeout` - Connection timeout (defaults: 10 seconds, can be changed in runtime)
-* `tarantool.retry_count` - Count of retries for connecting (defaults: 1, can be changed in runtime)
+* `tarantool.retry_count` - Amount of attempts to connect (defaults: 1, can be changed in runtime)
 * `tarantool.retry_sleep` - Sleep between connecting retries (defaults: 0.1 second, can be changed in runtime)
 * `tarantool.request_timeout` - Read/write timeout for requests (defaults: 10 second, can be changed in runtime)
 
diff --git a/src/tarantool.c b/src/tarantool.c
index c3ab7a4..389f176 100644
--- a/src/tarantool.c
+++ b/src/tarantool.c
@@ -257,6 +257,11 @@ static int __tarantool_connect(tarantool_object *t_obj) {
 	double_to_ts(INI_FLT("retry_sleep"), &sleep_time);
 	char *err = NULL;
 
+	if (count < 1) {
+		THROW_EXC("tarantool.retry_count should be 1 or above");
+		return FAILURE;
+	}
+
 	if (t_obj->is_persistent) {
 		if (!obj->persistent_id)
 			obj->persistent_id = pid_pzsgen(obj->host, obj->port,
diff --git a/test/CreateTest.php b/test/CreateTest.php
index f49310b..7ece97e 100644
--- a/test/CreateTest.php
+++ b/test/CreateTest.php
@@ -163,5 +163,33 @@ public static function provideGoodCredentials()
             ['guest', null],
         ];
     }
+
+    public function test_10_zero_retry_exception() {
+        /* A connection to call iproto_connect_count(). */
+        $tarantool = new Tarantool('localhost', self::$port, 'test', 'test');
+        $iproto_connect_count_before =
+            $tarantool->call('iproto_connect_count')[0][0];
+
+        $saved_retry_count = ini_get('tarantool.retry_count');
+        ini_set('tarantool.retry_count', 0);
+
+        $exp_err = 'tarantool.retry_count should be 1 or above';
+        try {
+            $c = new Tarantool('localhost', self::$port);
+            $c->connect();
+            $this->assertFalse(true);
+        } catch (Exception $e) {
+            $this->assertInstanceOf(TarantoolException::class, $e);
+            $this->assertEquals($exp_err, $e->getMessage());
+        } finally {
+            ini_set('tarantool.retry_count', $saved_retry_count);
+        }
+
+        /* Verify that no connection attempts were performed. */
+        $iproto_connect_count_after =
+            $tarantool->call('iproto_connect_count')[0][0];
+        $this->assertEquals($iproto_connect_count_before,
+                            $iproto_connect_count_after);
+    }
 }
 
diff --git a/test/shared/box.lua b/test/shared/box.lua
index 83cb0d7..33838c8 100755
--- a/test/shared/box.lua
+++ b/test/shared/box.lua
@@ -180,5 +180,22 @@ function test_6(...)
     return ...
 end
 
+iproto_connect_counter = 0
+function iproto_connect_count()
+    return iproto_connect_counter
+end
+
+box.session.on_connect(function()
+    -- box.session.type() was introduced in 1.7.4-370-g0bce2472b.
+    --
+    -- We're interested in iproto sessions, but it is okay for our
+    -- usage scenario to count replication and console sessions
+    -- too: we only see to a delta and AFAIK our testing harness
+    -- does not perform any background reconnections.
+    if box.session.type == nil or box.session.type() == 'binary' then
+        iproto_connect_counter = iproto_connect_counter + 1
+    end
+end)
+
 require('console').listen(os.getenv('ADMIN_PORT'))