* [tarantool-patches] [PATCH] tarantoolctl: return an error on enter to a dead socket. @ 2018-07-16 8:26 Serge Petrenko 2018-07-18 5:19 ` [tarantool-patches] " Konstantin Osipov 0 siblings, 1 reply; 4+ messages in thread From: Serge Petrenko @ 2018-07-16 8:26 UTC (permalink / raw) To: tarantool-patches; +Cc: 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) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket. 2018-07-16 8:26 [tarantool-patches] [PATCH] tarantoolctl: return an error on enter to a dead socket Serge Petrenko @ 2018-07-18 5:19 ` Konstantin Osipov 2018-07-18 9:19 ` Sergey Petrenko 0 siblings, 1 reply; 4+ messages in thread From: Konstantin Osipov @ 2018-07-18 5:19 UTC (permalink / raw) To: tarantool-patches; +Cc: Serge Petrenko * 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket. 2018-07-18 5:19 ` [tarantool-patches] " Konstantin Osipov @ 2018-07-18 9:19 ` Sergey Petrenko 2018-08-03 8:52 ` Sergey Petrenko 0 siblings, 1 reply; 4+ messages in thread From: Sergey Petrenko @ 2018-07-18 9:19 UTC (permalink / raw) To: Konstantin Osipov, tarantool-patches 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) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] tarantoolctl: return an error on enter to a dead socket. 2018-07-18 9:19 ` Sergey Petrenko @ 2018-08-03 8:52 ` Sergey Petrenko 0 siblings, 0 replies; 4+ messages in thread From: Sergey Petrenko @ 2018-08-03 8:52 UTC (permalink / raw) To: tarantool-patches; +Cc: Konstantin Osipov [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-03 8:52 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-16 8:26 [tarantool-patches] [PATCH] tarantoolctl: return an error on enter to a dead socket Serge Petrenko 2018-07-18 5:19 ` [tarantool-patches] " Konstantin Osipov 2018-07-18 9:19 ` Sergey Petrenko 2018-08-03 8:52 ` Sergey Petrenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox