* [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
| 19 ++++++++++++-------
test/app-tap/tarantoolctl.test.lua | 29 ++++++++++++++++++++++++++++-
2 files changed, 40 insertions(+), 8 deletions(-)
--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