From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 550AC29483 for ; Fri, 3 Aug 2018 04:52:43 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gvVf5Q9aZ9r0 for ; Fri, 3 Aug 2018 04:52:43 -0400 (EDT) Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D81152947B for ; Fri, 3 Aug 2018 04:52:42 -0400 (EDT) From: Sergey Petrenko Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_A28C2FD5-A64D-4E01-A9C1-A83330D34213" Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: [tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket. Date: Fri, 3 Aug 2018 11:52:40 +0300 In-Reply-To: <865891b0-f41e-342c-6a02-309502ff1b8a@tarantool.org> References: <20180716082655.42311-1-sergepetrenko@tarantool.org> <20180718051953.GA31091@chai> <865891b0-f41e-342c-6a02-309502ff1b8a@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Konstantin Osipov --Apple-Mail=_A28C2FD5-A64D-4E01-A9C1-A83330D34213 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 18 =D0=B8=D1=8E=D0=BB=D1=8F 2018 =D0=B3., =D0=B2 12:19, Sergey = Petrenko =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > Hi! >=20 > 18.07.2018 8:19, Konstantin Osipov =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> * Serge Petrenko [18/07/16 15:14]: >>=20 >>> 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. >>>=20 >>> Closes #3364 >>> extra/dist/tarantoolctl.in | 19 ++++++++++++------- >>> test/app-tap/tarantoolctl.test.lua | 29 = ++++++++++++++++++++++++++++- >>> 2 files changed, 40 insertions(+), 8 deletions(-) >>>=20 >>> + local status, err =3D 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=E2=80=99t leave a unix = socket after its exit, so it=E2=80=99s 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=E2=80=99t make it work. So all I have is a simple function that is executed in tx thread and=20 calls evio_service_stop() on binary service, which, in turn, unlinks the = socket. I don=E2=80=99t know if it=E2=80=99s 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 =3D 'script.control' > console_sock =3D 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 =3D string.format( - "require('console').connect('%s', { connect_timeout =3D %s })", - console_sock, TIMEOUT_INFINITY - ) - - console.on_start(function(self) self:eval(cmd) end) + local status, ret + console.on_start(function(self) + status, ret =3D pcall(console.connect, console_sock, + {connect_timeout =3D TIMEOUT_INFINITY}) + if not status then + log.error("Can't connect to %s (%s)", console_sock_path, = ret) + self.running =3D false + end + end) console.on_client_disconnect(function(self) self.running =3D false = end) console.start() + if not status then + return 1 + end return 0 end =20 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); } =20 + +/** + * 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); =20 +void +iproto_stop_listen(void); + void iproto_set_msg_max(int iproto_msg_max); =20 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 =20 +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 =3D 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 =20 local test =3D tap.test('tarantoolctl') -test:plan(6) +test:plan(7) =20 -- basic start/stop test -- must be stopped afterwards @@ -250,6 +258,44 @@ do end end =20 +-- check enter +do + local dir =3D fio.tempdir() + + local code =3D [[ box.cfg{} ]] + create_script(dir, 'script.lua', code) + + local status, err =3D 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 =3D 'script.control' + console_sock =3D 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 =3D=3D false then + print(("Error: %s"):format(err)) + os.exit() + end +end + -- check basic help do local dir =3D fio.tempdir() --=20 2.15.2 (Apple Git-101.1)= --Apple-Mail=_A28C2FD5-A64D-4E01-A9C1-A83330D34213 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

18 =D0=B8=D1=8E=D0=BB=D1=8F= 2018 =D0=B3., =D0=B2 12:19, Sergey Petrenko <sergepetrenko@tarantool.org> =D0=BD=D0=B0=D0=BF=D0=B8=D1= =81=D0=B0=D0=BB(=D0=B0):

Hi!

18.07.2018 8:19, Konstantin Osipov = =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
* 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 =3D 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=E2=80=99t leave a unix socket after its = exit, so it=E2=80=99s 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=E2=80=99t 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=E2=80=99t know if it=E2=80=99s = 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 =3D = 'script.control'
        console_sock = =3D 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 =3D = string.format(
-      =   "require('console').connect('%s', { connect_timeout =3D %s = })",
-        console_sock, = TIMEOUT_INFINITY
-    )
-
-    console.on_start(function(self) = self:eval(cmd) end)
+    local status, = ret
+    console.on_start(function(self)
+        status, ret =3D = pcall(console.connect, console_sock,
+      =         {connect_timeout =3D = TIMEOUT_INFINITY})
+        if = not status then
+          =   log.error("Can't connect to %s (%s)", console_sock_path, = ret)
+          =   self.running =3D false
+      =   end
+    end)
    =  console.on_client_disconnect(function(self) self.running =3D 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 =3D 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 =3D 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 =3D = fio.tempdir()
+
+    local = code =3D [[ box.cfg{} ]]
+  =   create_script(dir, 'script.lua', code)
+
+    local status, err =3D 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 =3D = 'script.control'
+          =   console_sock =3D 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 =3D=3D false then
+        print(("Error: = %s"):format(err))
+      =   os.exit()
+    end
+end
+
 -- check basic = help
 do
     local dir = =3D fio.tempdir()
-- 
2.15.2 (Apple = Git-101.1)= --Apple-Mail=_A28C2FD5-A64D-4E01-A9C1-A83330D34213--