[tarantool-patches] [PATCH] tarantoolctl: return an error on enter to a dead socket.

[tarantool-patches] [PATCH] tarantoolctl: return an error on enter to a dead socket.

[Serge Petrenko]

Tarantoolctl enter didn't check whether connection to a socket was established
if a socket file existed. It just executed a local console.
Fix this by adding a check and an error, also add a test case.

Closes #3364
---
https://github.com/tarantool/tarantool/issues/3364
https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3364-tarantoolctl-enter-fix

 extra/dist/tarantoolctl.in         | 19 ++++++++++++-------
 test/app-tap/tarantoolctl.test.lua | 29 ++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 2dd5d74ef..8085ede63 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -638,15 +638,20 @@ local function enter()
         end
         return 1
     end
-
-    local cmd = string.format(
-        "require('console').connect('%s', { connect_timeout = %s })",
-        console_sock, TIMEOUT_INFINITY
-    )
-
-    console.on_start(function(self) self:eval(cmd) end)
+    local status, ret
+    console.on_start(function(self)
+        pcall(console.connect, console_sock,
+              {connect_timeout = TIMEOUT_INFINITY})
+        if not status then
+            log.error("Can't connect to %s (%s)", console_sock_path, ret)
+            self.running = false
+        end
+    end)
     console.on_client_disconnect(function(self) self.running = false end)
     console.start()
+    if not status then
+        return 1
+    end
     return 0
 end
 
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 6946c8312..f40a90c55 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -150,7 +150,7 @@ local function check_ok(test, dir, cmd, args, e_res, e_stdout, e_stderr)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(6)
+test:plan(7)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -250,6 +250,33 @@ do
     end
 end
 
+-- check enter
+do
+    local dir = fio.tempdir()
+
+    local code = [[ box.cfg{} ]]
+    create_script(dir, 'script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check error codes in case of enter", function(test_i)
+            test_i:plan(6)
+            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
+            check_ok(test_i, dir, 'start', 'script', 0)
+            os.execute(("kill $(cat %s/script.pid)"):format(dir))
+            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
+            check_ok(test_i, dir, 'stop','script', 0)
+        end)
+    end)
+
+    cleanup_instance(dir, 'script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        prit(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 -- check basic help
 do
     local dir = fio.tempdir()
-- 
2.15.2 (Apple Git-101.1)

[tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket.

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/07/16 15:14]:

> Tarantoolctl enter didn't check whether connection to a socket was established
> if a socket file existed. It just executed a local console.
> Fix this by adding a check and an error, also add a test case.
> 
> Closes #3364

>  extra/dist/tarantoolctl.in         | 19 ++++++++++++-------
>  test/app-tap/tarantoolctl.test.lua | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> +    local status, err = pcall(function()
> +        test:test("check error codes in case of enter", function(test_i)
> +            test_i:plan(6)
> +            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
> +            check_ok(test_i, dir, 'start', 'script', 0)
> +            os.execute(("kill $(cat %s/script.pid)"):format(dir))

I don't understand how this works. Simply sending
SIGTERM to tarantool would should it down gracefully and remove
the socket. If it doesn't, it's a bug. You should use SIGKILL if
you wanted to create a dead socket. But then again why do you need
to start an instance in order to get a dead socket? Can't you make
it simpler and create a dead socket with fio API?

> +            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
> +            check_ok(test_i, dir, 'stop','script', 0)
> +        end)
> +    end)

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket.

[Sergey Petrenko]

Hi!

18.07.2018 8:19, Konstantin Osipov пишет:
> * Serge Petrenko <sergepetrenko@tarantool.org> [18/07/16 15:14]:
>
>> Tarantoolctl enter didn't check whether connection to a socket was established
>> if a socket file existed. It just executed a local console.
>> Fix this by adding a check and an error, also add a test case.
>>
>> Closes #3364
>>   extra/dist/tarantoolctl.in         | 19 ++++++++++++-------
>>   test/app-tap/tarantoolctl.test.lua | 29 ++++++++++++++++++++++++++++-
>>   2 files changed, 40 insertions(+), 8 deletions(-)
>>
>> +    local status, err = pcall(function()
>> +        test:test("check error codes in case of enter", function(test_i)
>> +            test_i:plan(6)
>> +            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
>> +            check_ok(test_i, dir, 'start', 'script', 0)
>> +            os.execute(("kill $(cat %s/script.pid)"):format(dir))
> I don't understand how this works. Simply sending
> SIGTERM to tarantool would should it down gracefully and remove
> the socket. If it doesn't, it's a bug. You should use SIGKILL if
Doing
   tarantoolctl start testctl
   kill (tarantool instance pid)
Leaves the socket intact on my pc.
tarantoolctl stop also first explicitly deletes the socket
file and then sends SIGTERM to the instance.
Don't know if it's ok.

> you wanted to create a dead socket. But then again why do you need
> to start an instance in order to get a dead socket? Can't you make
> it simpler and create a dead socket with fio API?
Do you mean create a file with fio.open(sock, 'O_CREAT')?
It won't be a 'real' socket, but I tried it, and the tests seem to
work.
Because AFAIU there is no way to create a unix socket without
starting to listen on it.
That's what I'm doing now:
         test_i:plan(4)
         check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect 
to")
         local console_sock = 'script.control'
         console_sock = fio.pathjoin(fio.dirname(dir), console_sock)
         fio.open(console_sock, 'O_CREAT')
         check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect 
to")
         fio.unlink(console_sock)

[tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket.

[Sergey Petrenko]

[-- Attachment #1: Type: text/plain, Size: 8238 bytes --]



> 18 июля 2018 г., в 12:19, Sergey Petrenko <sergepetrenko@tarantool.org> написал(а):
> 
> Hi!
> 
> 18.07.2018 8:19, Konstantin Osipov пишет:
>> * Serge Petrenko <sergepetrenko@tarantool.org> [18/07/16 15:14]:
>> 
>>> Tarantoolctl enter didn't check whether connection to a socket was established
>>> if a socket file existed. It just executed a local console.
>>> Fix this by adding a check and an error, also add a test case.
>>> 
>>> Closes #3364
>>>  extra/dist/tarantoolctl.in         | 19 ++++++++++++-------
>>>  test/app-tap/tarantoolctl.test.lua | 29 ++++++++++++++++++++++++++++-
>>>  2 files changed, 40 insertions(+), 8 deletions(-)
>>> 
>>> +    local status, err = pcall(function()
>>> +        test:test("check error codes in case of enter", function(test_i)
>>> +            test_i:plan(6)
>>> +            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
>>> +            check_ok(test_i, dir, 'start', 'script', 0)
>>> +            os.execute(("kill $(cat %s/script.pid)"):format(dir))
>> I don't understand how this works. Simply sending
>> SIGTERM to tarantool would should it down gracefully and remove
>> the socket. If it doesn't, it's a bug. You should use SIGKILL if
> Doing
>   tarantoolctl start testctl
>   kill (tarantool instance pid)
> Leaves the socket intact on my pc.
> tarantoolctl stop also first explicitly deletes the socket
> file and then sends SIGTERM to the instance.
> Don't know if it's ok.

Hi! As we discussed verbally, tarantool shouldn’t leave a unix socket after its exit, so it’s a bug.
I fixed it and rebased the patch on top of latest 1.10
The thing is that iproto thread should unlink the file (if any) upon its stop, but we never stop
iproto thread. I tried to implement a proper stop function, like the one I wrote for replication_shutdown(),
but couldn’t make it work.
So all I have is a simple function that is executed in tx thread and 
calls evio_service_stop() on binary service, which, in turn, unlinks the socket.
I don’t know if it’s ok to call evio_service_stop() from another thread, so I need your comments.

I also added some test cases to check for correct creation and deletion of unix socket under tarantoolctl.
Please see new diff below.

>> you wanted to create a dead socket. But then again why do you need
>> to start an instance in order to get a dead socket? Can't you make
>> it simpler and create a dead socket with fio API?
> Do you mean create a file with fio.open(sock, 'O_CREAT')?
> It won't be a 'real' socket, but I tried it, and the tests seem to
> work.
> Because AFAIU there is no way to create a unix socket without
> starting to listen on it.
> That's what I'm doing now:
>         test_i:plan(4)
>         check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
>         local console_sock = 'script.control'
>         console_sock = fio.pathjoin(fio.dirname(dir), console_sock)
>         fio.open(console_sock, 'O_CREAT')
>         check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
>         fio.unlink(console_sock)

 extra/dist/tarantoolctl.in         | 19 +++++++++------
 src/box/box.cc                     |  2 ++
 src/box/iproto.cc                  | 16 +++++++++++++
 src/box/iproto.h                   |  3 +++
 test/app-tap/tarantoolctl.test.lua | 48 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index f0529734f..8d52a4d9c 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -638,15 +638,20 @@ local function enter()
         end
         return 1
     end
-
-    local cmd = string.format(
-        "require('console').connect('%s', { connect_timeout = %s })",
-        console_sock, TIMEOUT_INFINITY
-    )
-
-    console.on_start(function(self) self:eval(cmd) end)
+    local status, ret
+    console.on_start(function(self)
+        status, ret = pcall(console.connect, console_sock,
+              {connect_timeout = TIMEOUT_INFINITY})
+        if not status then
+            log.error("Can't connect to %s (%s)", console_sock_path, ret)
+            self.running = false
+        end
+    end)
     console.on_client_disconnect(function(self) self.running = false end)
     console.start()
+    if not status then
+        return 1
+    end
     return 0
 end
 
diff --git a/src/box/box.cc b/src/box/box.cc
index ee12d5738..adc3204bf 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1606,6 +1606,8 @@ box_free(void)
 		tuple_free();
 		port_free();
 #endif
+		/* If listening on a unix socket, unlink it. */
+		iproto_stop_listen();
 		sequence_free();
 		gc_free();
 		engine_shutdown();
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index bb7d2b868..0bd3f7a4f 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2015,6 +2015,22 @@ iproto_listen(const char *uri)
 	iproto_do_cfg(&cfg_msg);
 }
 
+
+/**
+ * Stop binary service.
+ *
+ * We don't have a proper iproto shutdown, iproto thread
+ * just dies and never stops listening on socket. This leaves
+ * dead unix sockets after tarantool. So we have to manually
+ * call evio_service_stop() to unink unix socket, if any.
+ * When iproto_shutdown() will be implemented, this function
+ * won't be needed anymore
+ */
+void
+iproto_stop_listen(void) {
+	evio_service_stop(&binary);
+}
+
 size_t
 iproto_mem_used(void)
 {
diff --git a/src/box/iproto.h b/src/box/iproto.h
index b9a6cf8f7..304af3b67 100644
--- a/src/box/iproto.h
+++ b/src/box/iproto.h
@@ -77,6 +77,9 @@ iproto_init();
 void
 iproto_listen(const char *uri);
 
+void
+iproto_stop_listen(void);
+
 void
 iproto_set_msg_max(int iproto_msg_max);
 
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 6946c8312..1ba31ccf6 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -113,6 +113,14 @@ local function tctl_wait(dir, name)
     end
 end
 
+local function wait_delete(path)
+    if path then
+        while fio.path.exists(path) do
+            fiber.sleep(0.001)
+        end
+    end
+end
+
 local function tctl_command(dir, cmd, args, name)
     local pid = nil
     if not fio.stat(fio.pathjoin(dir, '.tarantoolctl')) then
@@ -150,7 +158,7 @@ local function check_ok(test, dir, cmd, args, e_res, e_stdout, e_stderr)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(6)
+test:plan(7)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -250,6 +258,44 @@ do
     end
 end
 
+-- check enter
+do
+    local dir = fio.tempdir()
+
+    local code = [[ box.cfg{} ]]
+    create_script(dir, 'script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check error codes in case of enter", function(test_i)
+            test_i:plan(10)
+            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
+            local console_sock = 'script.control'
+            console_sock = fio.pathjoin(dir, console_sock)
+            test_i:is(fio.path.exists(console_sock), false, "directory clean")
+            check_ok(test_i, dir, 'start', 'script', 0)
+            tctl_wait(dir, 'script')
+            test_i:is(fio.path.exists(console_sock), true,
+                      "unix socket created")
+            check_ok(test_i, dir, 'stop', 'script', 0)
+            --wait_delete(console_sock)
+            test_i:is(fio.path.exists(console_sock), false,
+	              "remove unix socket upon exit")
+            fio.open(console_sock, 'O_CREAT')
+	    test_i:is(fio.path.exists(console_sock), true, "file created")
+            check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to")
+            fio.unlink(console_sock)
+        end)
+    end)
+
+    cleanup_instance(dir, 'script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        print(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 -- check basic help
 do
     local dir = fio.tempdir()
-- 
2.15.2 (Apple Git-101.1)

[-- Attachment #2: Type: text/html, Size: 15442 bytes --]