Re: [tarantool-patches] [PATCH v5 2/3] lua: patch os.exit() to execute on_shutdown triggers.

[Serge Petrenko <sergepetrenko@tarantool.org>]

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

Hi! Thankyou for review. I addressed your comments.

> 28 янв. 2019 г., в 11:45, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Fri, Jan 25, 2019 at 06:41:33PM +0300, Serge Petrenko wrote:
>> Note that the triggers will not be run if tarantool receives a fatal
>> signal: `SIGSEGV`, `SIGABORT` or any signal causing immediate program
>> termination.
>> 
> 
>> Closes #1607
> 
> Should go before the TarantoolBot request.
> 

Fixed.

>> ---
>> extra/exports    |  1 +
>> src/lua/init.lua |  7 +++++++
>> src/main.cc      | 10 ++++++----
>> src/main.h       |  3 +++
>> 4 files changed, 17 insertions(+), 4 deletions(-)
> 
> A test is missing. Please add.
> 

Added a test

>> 
>> 
>> 	is_shutting_down = true;
>> +	exit_code = code;
>> 	fiber_wakeup(on_shutdown_fiber);
> 
> os.exit() should exit even if the fiber doesn't yield, but the following
> code will hang, which is unexpected. Please fix.
> 
> 	os.exit()
> 	while true do end

Fixed

I pushed the fixes on the branch
https://github.com/tarantool/tarantool/tree/sp/gh-1607-on-exit-triggers
Incremental diff is below.

diff --git a/src/lua/init.lua b/src/lua/init.lua
index a961db328..9fd56f483 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -52,9 +52,14 @@ dostring = function(s, ...)
     return chunk(...)
 end
 
+local fiber = require("fiber")
 os.exit = function(code)
     code = (type(code) == 'number') and code or 0
     ffi.C.tarantool_exit(code)
+    -- Make sure we yield even if the code after
+    -- os.exit() never yields. After on_shutdown
+    -- fiber completes, we will never wake up again.
+    while true do fiber.yield() end
 end
 
 local function uptime()
diff --git a/test/box/misc.result b/test/box/misc.result
index 6912915c1..97189ecbb 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1321,3 +1321,92 @@ box.space.shutdown:select{}
 box.space.shutdown:drop()
 ---
 ...
+-- Check that os.exit invokes triggers
+fiber = require("fiber")
+---
+...
+test_run:cmd("create server test with script='box/proxy.lua'")
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+logfile = test_run:eval("test", "box.cfg.log")[1]
+---
+...
+test_run:cmd("stop server test")
+---
+- true
+...
+-- clean up any leftover logs
+require("fio").unlink(logfile)
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+_ = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
+---
+...
+-- Check that we don't hang infinitely after os.exit()
+-- even if the following code doesn't yield.
+fiber = require("fiber")
+---
+...
+_ = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end end)
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+fiber.sleep(0.1)
+---
+...
+-- The server should be already stopped by os.exit(),
+-- but start doesn't work without a prior call to stop.
+test_run:cmd("stop server test")
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=true})
+---
+- on_shutdown 5
+...
+-- make sure we exited because of os.exit(), not a signal.
+test_run:grep_log('test', 'signal', nil, {noreset=true})
+---
+- null
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd("stop server test")
+---
+- true
+...
+test_run:cmd("cleanup server test")
+---
+- true
+...
+test_run:cmd("delete server test")
+---
+- true
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index f1c9d8e8c..18128b299 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -384,3 +384,33 @@ test_run:grep_log('default', 'on_shutdown 3', nil, {noreset=true})
 test_run:grep_log('default', 'on_shutdown 4', nil, {noreset=true})
 box.space.shutdown:select{}
 box.space.shutdown:drop()
+
+-- Check that os.exit invokes triggers
+fiber = require("fiber")
+test_run:cmd("create server test with script='box/proxy.lua'")
+test_run:cmd("start server test")
+logfile = test_run:eval("test", "box.cfg.log")[1]
+test_run:cmd("stop server test")
+-- clean up any leftover logs
+require("fio").unlink(logfile)
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+_ = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
+-- Check that we don't hang infinitely after os.exit()
+-- even if the following code doesn't yield.
+fiber = require("fiber")
+_ = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end end)
+test_run:cmd("switch default")
+fiber.sleep(0.1)
+-- The server should be already stopped by os.exit(),
+-- but start doesn't work without a prior call to stop.
+test_run:cmd("stop server test")
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=true})
+-- make sure we exited because of os.exit(), not a signal.
+test_run:grep_log('test', 'signal', nil, {noreset=true})
+test_run:cmd("switch default")
+test_run:cmd("stop server test")
+test_run:cmd("cleanup server test")
+test_run:cmd("delete server test")

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