Implement delayed server shutdown with cancelation #4664

nerzhul · 2016-10-25T06:54:58Z

Add delayed server shutdown with periodic messages.

It's backported partially from epixel fork. It needs some improvements

Correct cancellation handling (wrong message)
Send the shutdown delay when a client connects

nerzhul · 2016-10-25T06:55:20Z

This will override existing #3580 which should be closed after that

rubenwardy · 2016-10-25T13:47:37Z

builtin/game/chatcommands.lua

@@ -815,12 +815,18 @@ core.register_chatcommand("days", {
 })

 core.register_chatcommand("shutdown", {
+    params = "<delay_in_seconds(0..inf)> | Leave blank for normal shutdown",


Should accept eg: 3h2m
See the other pr for code kaeza volunteered

Interesting parsing @kaeza did you agree adding the following code to core ?

local unit_to_secs = { s = 1, m = 60, h = 3600, D = 86400, W = 604800, M = 2592000, Y = 31104000, [""] = 1, } local function parse_time(t) --> secs local secs = 0 for num, unit in t:gmatch("(%d+)([smhDWMY]?)") do secs = secs + (tonumber(num) * (unit_to_secs[unit] or 1)) end return secs end

the only problem i see with the previous code is that we can do 3h2m3h and this will do 6h2m. Maybe a more stricter regexp (%dh)?(%dm)? ... is better

maybe this improvement can be added in another PR later, to have kiss PR instead of huge PRs :) as here it require to add new features to minetest lua core

indentation problem

@nerzhul That kind of thing is not possible with Lua patterns. It's not a full-fledged RE engine.

It's not worth the effort to restrict anything as long as there's some feedback noting the computed value, but if it's really important, here's a somewhat better implementation:

local unit_to_secs = { s = 1, m = 60, h = 3600, D = 86400, W = 604800, M = 2592000, Y = 31104000, } local function parse_time(t) local tm = { } for k in pairs(unit_to_secs) do tm[k] = 0 end for num, unit in t:gmatch("(%d+)([smhDWMY]?)") do if unit == "" then unit = "s" end tm[unit] = tonumber(num) end local total = 0 for k, v in pairs(tm) do total = total + unit_to_secs[k]*v end return total end assert(parse_time("1m2m") == 120)

rubenwardy · 2016-10-25T13:48:07Z

doc/lua_api.txt

-    and `reconnect` == true displays a reconnect button.
+* `minetest.request_shutdown([message],[reconnect],[delay])`: request for server shutdown. Will display `message` to clients,
+    `reconnect` == true displays a reconnect button,
+    `delay` permit to add an optionnal delay before shutdown


rubenwardy · 2016-10-25T13:52:23Z

doc/lua_api.txt

+* `minetest.request_shutdown([message],[reconnect],[delay])`: request for server shutdown. Will display `message` to clients,
+    `reconnect` == true displays a reconnect button,
+    `delay` permit to add an optionnal delay before shutdown
+        negative delay cancel the current active shutdown


Would prefer minetest.cancel_shutdown_requests for nicer code.

A more engineered solution would have ids like in the hud api so that a mod only cancels their own requests, but that's over engineered I feel.

i think this is very very overengineered for a shutdown. you should only have core or one mod at maximum to do this action on your server else its the chaos on your shutdown behaviour

Just in case of miscommunication here, my first paragraph meant adding the equivalent of:

function minetest.cancel_shutdown_requests() minetest.request_shutdown("", false, -1) end

I added the requested change to builtin, are you okay with this ?

rubenwardy · 2016-10-25T13:52:54Z

src/network/serverpackethandler.cpp

@@ -660,6 +660,14 @@ void Server::handleCommand_Init2(NetworkPacket* pkt)
 	if (protocol_version <= 22) {
 		m_clients.event(pkt->getPeerId(), CSE_SetClientReady);
 		m_script->on_joinplayer(playersao);
+		// Send shutdown timer if shutdown has been sheduled


nerzhul · 2016-10-25T14:20:09Z

@rubenwardy typos were fix thanks

SmallJoker · 2016-10-25T16:01:49Z

Do we really need a C++ implementation of this? This is currently also doable with Lua, so this looks a bit overengineered to me.

nerzhul · 2016-10-25T16:04:41Z

@SmallJoker yes it's better to do this in C++ as it's server shutdown. MT is not a full Lua game.
Shutdown process is sensible and should be in pure C++ to avoid problems between the Lua stack and the core stack as Lua is owned by C++.
Also i prefer the C++ approach as it already exists properly and have higher Lua features is better than doing everything in lua

nerzhul · 2016-10-25T17:24:51Z

Add my own approval

sofar · 2016-10-25T18:53:31Z

I agree that a C++ implementation in this case is probably wanted - server operators are likely to schedule downtime and want it to be robust and potentially survive mod errors or hangs. There may be plenty of cases where that will be difficult or unlikely, but it still helps.

This is a lot of code but fairly trivial, it looks OK to me 👍.

kwolekr · 2016-10-25T19:21:05Z

doc/lua_api.txt

+* `minetest.request_shutdown([message],[reconnect],[delay])`: request for server shutdown. Will display `message` to clients,
+    `reconnect` == true displays a reconnect button,
+    `delay` permit to add an optional delay before shutdown
+        negative delay cancel the current active shutdown


cancels. Not cancel.

est31 · 2016-10-25T19:07:20Z

src/server.cpp

+		std::wstringstream ws;
+
+		ws << L"*** Server shutting down in "
+		   << duration_to_string(round(m_shutdown_timer)).c_str()


indentation problem

est31 · 2016-10-25T19:07:34Z

src/server.cpp

+					std::wstringstream ws;
+
+					ws << L"*** Server shutting down in "
+					   << duration_to_string(round(m_shutdown_timer - dtime)).c_str()


indentation problem

exact, strange that changed... fixing the two issues now

est31 · 2016-10-25T19:29:00Z

Okay, but is any server owner actually wanting to use this? I mean, how many server owners currently implement this or a similar thing in lua?

nerzhul · 2016-10-25T19:30:17Z

@est31 didn't see this feature on any server i went, generally they sent a broadcast message manually and then restart, or they wait for the next crash :s

kwolekr · 2016-10-25T19:21:57Z

doc/lua_api.txt

-    and `reconnect` == true displays a reconnect button.
+* `minetest.request_shutdown([message],[reconnect],[delay])`: request for server shutdown. Will display `message` to clients,
+    `reconnect` == true displays a reconnect button,
+    `delay` permit to add an optional delay before shutdown


"permit to add"?
What is the delay measured in? Seconds? Milliseconds? Years?

kwolekr · 2016-10-25T19:25:04Z

src/script/lua_api/l_server.cpp

@@ -33,7 +33,8 @@ int ModApiServer::l_request_shutdown(lua_State *L)
 	NO_MAP_LOCK_REQUIRED;
 	const char *msg = lua_tolstring(L, 1, NULL);
 	bool reconnect = lua_toboolean(L, 2);
-	getServer(L)->requestShutdown(msg ? msg : "", reconnect);
+	float seconds_before_shutdown = luaL_checknumber(L, 3);


luaL_checknumber()?
I thought you said this parameter was optional.

is lua_tonumber the right function ?

I would hope that you actually know what each of these functions do...

@nerzhul luaL_optnumber.

kwolekr · 2016-10-25T19:26:25Z

src/server.cpp

+		SendChatMessage(PEER_ID_INEXISTENT, ws.str());
+	}
+	// Positive delay, delay the shutdown
+	else if (delay > 0.0f) {


Incorrect code style here

what is wrong there ?

} and else on separate lines, they should be on the same line. You can put the comment after the {

kwolekr · 2016-10-25T19:32:10Z

You know, I kind of agree with est31. Does anybody actually want this feature?
Are we sure it's not just adding unnecessary bloat? This can trivially be done with a mod.
👎 on concept

est31 · 2016-10-25T19:35:23Z

src/server.cpp

+		1, 2, 3, 4, 5, 10, 15, 20, 25, 30, 45, 60, 120, 180, 300, 600, 1200, 1800, 3600
+	};
+
+	if (m_shutdown_timer > 0.0f) {


I'm not really sure how this is more reliable than if done in lua. If lua hangs, doesn't this code get not executed as well?

if lua hang this timer is on server loop the server can shutdown properly.

Its the same function that calls m_env->step, the function which calls all the ABMs and all other lua related stuff. So no, if lua hangs, this code isn't reached either.

yes ofc you are right because we trigger an exception

nerzhul · 2016-10-25T19:38:28Z

the problem is we can do everything in lua, but sensitive informations like shutdown should not be exposed directly to mods.
Adding new lightweight useful features to core which are available and cheap for users is not a problem.

nerzhul · 2016-10-25T19:48:33Z

Last other problem with the politic to let Lua do everything and everything should be a mod is you offer users and empty shell (don't know if it's english for french "coquille vide"), users want to spawn a server and benefit from features directly and after if those base features are interesting will look for mods. An empty shell is not interesting for users, they want base and if base is interesting, mod it.

kwolekr · 2016-10-25T20:43:47Z

What's wrong with adding the mod that does this to minetest_game then?

kaeza · 2016-10-25T21:01:14Z

I agree with @SmallJoker: do you really need to use C++ here?

If there's any problem with request_shutdown, why is that even exposed to Lua? What does it change if we call it now or schedule it to be called at a later time?

kwolekr · 2016-10-26T04:27:27Z

You can't give approval to yourself, nerzhul.

Zeno- · 2016-10-26T04:34:30Z

I agree with those saying that this is most likely not needed

est31 · 2016-10-26T14:13:07Z

You can give your own approval.

nerzhul · 2016-10-27T07:00:54Z

Closing as nobody except @sofar is interested to have features in core available to everybody without cost.
If you want to port it in Lua okay, but add it to builtin, not minetest_game, no need to recode this 50 times to do the same thing.

ShadowNinja · 2017-04-15T18:18:49Z

@nerzhul: We agreed to this on IRC (http://irc.minetest.net/minetest-dev/2017-04-15#i_4876826) and assigned you. Please rebase and merge this, and close #3580 too when you're done.

nerzhul · 2017-04-15T18:58:43Z

rebased & tested, @ShadowNinja @sofar @Ekdohibs please permit a merge :)

celeron55 · 2017-04-15T19:57:32Z

builtin/game/chatcommands.lua

@@ -763,14 +763,20 @@ core.register_chatcommand("days", {

 core.register_chatcommand("shutdown", {
 	description = "Shutdown server",
-	params = "[reconnect] [message]",
+	params = "[delay_in_seconds(0..inf)] [reconnect] [message]",


I think delay=-1 cancels the shutdown; this should say something like:

[delay_in_seconds(0..inf) or -1 for cancel] [reconnect] [message]

celeron55 · 2017-04-15T20:19:41Z

Anyway +1 once the user knows how to cancel a delayed shutdown via chat.

nerzhul added the WIP The PR is still being worked on by its author and not ready yet. label Oct 25, 2016

nerzhul added this to the 0.4.15 milestone Oct 25, 2016

nerzhul mentioned this pull request Oct 25, 2016

Added delayed shutdown feature #3580

Closed

nerzhul force-pushed the delayed_shutdown branch from 29db2be to 5f01bc9 Compare October 25, 2016 13:25

nerzhul added Enhancement @ Server / Client / Env. and removed WIP The PR is still being worked on by its author and not ready yet. labels Oct 25, 2016

rubenwardy reviewed Oct 25, 2016

View reviewed changes

nerzhul force-pushed the delayed_shutdown branch from 5f01bc9 to f4f0b06 Compare October 25, 2016 14:01

nerzhul force-pushed the delayed_shutdown branch from f4f0b06 to 17fe1da Compare October 25, 2016 14:26

nerzhul added the One approval ✅ ◻️ label Oct 25, 2016

sofar added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Oct 25, 2016

kwolekr reviewed Oct 25, 2016

View reviewed changes

est31 suggested changes Oct 25, 2016

View reviewed changes

nerzhul force-pushed the delayed_shutdown branch 3 times, most recently from 2335f64 to c15f58f Compare October 25, 2016 19:28

kwolekr reviewed Oct 25, 2016

View reviewed changes

nerzhul force-pushed the delayed_shutdown branch from c15f58f to 7e962e7 Compare October 25, 2016 19:31

est31 reviewed Oct 25, 2016

View reviewed changes

nerzhul force-pushed the delayed_shutdown branch from 7e962e7 to fa7c4ef Compare October 25, 2016 19:44

nerzhul force-pushed the delayed_shutdown branch from fa7c4ef to f1ebaba Compare October 25, 2016 21:02

kwolekr added One approval ✅ ◻️ and removed >= Two approvals ✅ ✅ labels Oct 26, 2016

nerzhul added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Oct 26, 2016

nerzhul closed this Oct 27, 2016

ShadowNinja assigned nerzhul Apr 15, 2017

ShadowNinja reopened this Apr 15, 2017

nerzhul force-pushed the delayed_shutdown branch 3 times, most recently from 3a2d10b to e294c52 Compare April 15, 2017 18:48

celeron55 reviewed Apr 15, 2017

View reviewed changes

Implement delayed server shutdown with cancelation

7f2cd2e

nerzhul force-pushed the delayed_shutdown branch from e294c52 to 7f2cd2e Compare April 15, 2017 20:40

nerzhul merged commit 34d32ce into minetest:master Apr 15, 2017

nerzhul deleted the delayed_shutdown branch June 8, 2017 19:39

Implement delayed server shutdown with cancelation #4664

Implement delayed server shutdown with cancelation #4664

Conversation

nerzhul commented Oct 25, 2016 • edited

nerzhul commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul commented Oct 25, 2016

SmallJoker commented Oct 25, 2016

nerzhul commented Oct 25, 2016 • edited

nerzhul commented Oct 25, 2016

sofar commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

est31 commented Oct 25, 2016

nerzhul commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul Oct 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwolekr commented Oct 25, 2016 • edited

Choose a reason for hiding this comment

nerzhul Oct 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul commented Oct 25, 2016

nerzhul commented Oct 25, 2016

kwolekr commented Oct 25, 2016

kaeza commented Oct 25, 2016 • edited

kwolekr commented Oct 26, 2016

Zeno- commented Oct 26, 2016

est31 commented Oct 26, 2016

nerzhul commented Oct 27, 2016

ShadowNinja commented Apr 15, 2017 • edited

nerzhul commented Apr 15, 2017

Choose a reason for hiding this comment

celeron55 commented Apr 15, 2017

nerzhul commented Oct 25, 2016 •

edited

nerzhul commented Oct 25, 2016 •

edited

nerzhul Oct 25, 2016 •

edited

kwolekr commented Oct 25, 2016 •

edited

nerzhul Oct 25, 2016 •

edited

kaeza commented Oct 25, 2016 •

edited

ShadowNinja commented Apr 15, 2017 •

edited