[Tarantool-patches] [PATCH] clear terminal state on panic

[Tarantool-patches] [PATCH] clear terminal state on panic

[Serge Petrenko]

The tarantool_free() call in the end of main() works all the time except
when we exit due to a panic. We need to clear terminal state in this
case also, so return to using atexit() to clear readline state.

Closes #4466
---
https://github.com/tarantool/tarantool/issues/4466
https://github.com/tarantool/tarantool/tree/sp/gh-4466-clear-rl-state

 src/main.cc | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/main.cc b/src/main.cc
index 0ff2213b6..014958a17 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -588,6 +588,28 @@ load_cfg()
 	box_cfg();
 }
 
+void
+free_rl_state(void)
+{
+	/* Same checks as in tarantool_free() */
+	if (getpid() != master_pid)
+		return;
+
+	if (!cord_is_main())
+		return;
+
+	/* tarantool_lua_free() was formerly reponsible for terminal reset,
+	 * but it is no longer called
+	 */
+	if (isatty(STDIN_FILENO)) {
+		/*
+		 * Restore terminal state. Doesn't hurt if exiting not
+		 * due to a signal.
+		 */
+		rl_cleanup_after_signal();
+	}
+}
+
 void
 tarantool_free(void)
 {
@@ -622,16 +644,6 @@ tarantool_free(void)
 #ifdef ENABLE_GCOV
 	__gcov_flush();
 #endif
-	/* tarantool_lua_free() was formerly reponsible for terminal reset,
-	 * but it is no longer called
-	 */
-	if (isatty(STDIN_FILENO)) {
-		/*
-		 * Restore terminal state. Doesn't hurt if exiting not
-		 * due to a signal.
-		 */
-		rl_cleanup_after_signal();
-	}
 	cbus_free();
 #if 0
 	/*
@@ -836,6 +848,8 @@ main(int argc, char **argv)
 		trigger_create(&break_loop_trigger, break_loop, NULL, NULL);
 		trigger_add(&box_on_shutdown, &break_loop_trigger);
 
+		atexit(free_rl_state);
+
 		if (!loop())
 			panic("%s", "can't init event loop");
 
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [19/11/26 18:17]:
> The tarantool_free() call in the end of main() works all the time except
> when we exit due to a panic. We need to clear terminal state in this
> case also, so return to using atexit() to clear readline state.

This comment belongs to the code, not just to the changeset
comment.

Nobody is going to read the changset comment when introducing
_exit() or moving it over to somewhere else.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Cyrill Gorcunov]

On Tue, Nov 26, 2019 at 11:56:20PM +0300, Konstantin Osipov wrote:
> * Serge Petrenko <sergepetrenko@tarantool.org> [19/11/26 18:17]:
> > The tarantool_free() call in the end of main() works all the time except
> > when we exit due to a panic. We need to clear terminal state in this
> > case also, so return to using atexit() to clear readline state.
> 
> This comment belongs to the code, not just to the changeset
> comment.
> 
> Nobody is going to read the changset comment when introducing
> _exit() or moving it over to somewhere else.

Should not we rather provide some tarantool_atexit() helper
from where we would call other cleanup and etc routines?
We already have tarantool_atfork() hook. Not a big deal
but while the code in question is being modified anyway.

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/11/27 19:12]:
> On Tue, Nov 26, 2019 at 11:56:20PM +0300, Konstantin Osipov wrote:
> > * Serge Petrenko <sergepetrenko@tarantool.org> [19/11/26 18:17]:
> > > The tarantool_free() call in the end of main() works all the time except
> > > when we exit due to a panic. We need to clear terminal state in this
> > > case also, so return to using atexit() to clear readline state.
> > 
> > This comment belongs to the code, not just to the changeset
> > comment.
> > 
> > Nobody is going to read the changset comment when introducing
> > _exit() or moving it over to somewhere else.
> 
> Should not we rather provide some tarantool_atexit() helper
> from where we would call other cleanup and etc routines?
> We already have tarantool_atfork() hook. Not a big deal
> but while the code in question is being modified anyway.

Why not, still the exit procedure is complicated because of
on_shutdown triggers, so it's important that if there are changes
to it, they are documented in the code, not in cs comments.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Cyrill Gorcunov]

On Thu, Nov 28, 2019 at 12:51:47AM +0300, Konstantin Osipov wrote:
> > 
> > Should not we rather provide some tarantool_atexit() helper
> > from where we would call other cleanup and etc routines?
> > We already have tarantool_atfork() hook. Not a big deal
> > but while the code in question is being modified anyway.
> 
> Why not, still the exit procedure is complicated because of
> on_shutdown triggers, so it's important that if there are changes
> to it, they are documented in the code, not in cs comments.

Yes, putting comment into the code why we're to restore the
terminal settings on panic is quite important.

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Serge Petrenko]

Hi! Thanks for your replies!

Fixed:

diff --git a/src/main.cc b/src/main.cc
index 014958a17..a1572b81e 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -589,7 +589,7 @@ load_cfg()
 }
 
 void
-free_rl_state(void)
+tarantool_atexit(void)
 {
        /* Same checks as in tarantool_free() */
        if (getpid() != master_pid)
@@ -848,7 +848,14 @@ main(int argc, char **argv)
                trigger_create(&break_loop_trigger, break_loop, NULL, NULL);
                trigger_add(&box_on_shutdown, &break_loop_trigger);
 
-               atexit(free_rl_state);
+               /*
+                * The call to tarantool_free() below, thanks to
+                * on_shutdown triggers, works all the time
+                * except when we panic. So leave the ever-
+                * necessary cleanups in atexit handler, which
+                * is executed always.
+                */
+               atexit(tarantool_atexit);
 
                if (!loop())
                        panic("%s", "can't init event loop");

--
Serge Petrenko
sergepetrenko@tarantool.org




> 28 нояб. 2019 г., в 1:31, Cyrill Gorcunov <gorcunov@gmail.com> написал(а):
> 
> On Thu, Nov 28, 2019 at 12:51:47AM +0300, Konstantin Osipov wrote:
>>> 
>>> Should not we rather provide some tarantool_atexit() helper
>>> from where we would call other cleanup and etc routines?
>>> We already have tarantool_atfork() hook. Not a big deal
>>> but while the code in question is being modified anyway.
>> 
>> Why not, still the exit procedure is complicated because of
>> on_shutdown triggers, so it's important that if there are changes
>> to it, they are documented in the code, not in cs comments.
> 
> Yes, putting comment into the code why we're to restore the
> terminal settings on panic is quite important.

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Cyrill Gorcunov]

On Fri, Nov 29, 2019 at 02:06:56PM +0300, Serge Petrenko wrote:
> Hi! Thanks for your replies!
>

I meant different, something like

void
tarantool_atexit(void)
{
	/*
	 * Terminal settings might be screwed
	 * when exit on panic, restore the
	 * normal one.
	 */
	free_rl_state(void);
}

...
	at_exit(tarantool_atexit);

This will allow us to extend tarantool_atexit()
with other cleanup routines if we need to.
Sounds reasonable?

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Serge Petrenko]

> 29 нояб. 2019 г., в 14:15, Cyrill Gorcunov <gorcunov@gmail.com> написал(а):
> 
> On Fri, Nov 29, 2019 at 02:06:56PM +0300, Serge Petrenko wrote:
>> Hi! Thanks for your replies!
>> 
> 
> I meant different, something like
> 
> void
> tarantool_atexit(void)
> {
> 	/*
> 	 * Terminal settings might be screwed
> 	 * when exit on panic, restore the
> 	 * normal one.
> 	 */
> 	free_rl_state(void);
> }
> 
> ...
> 	at_exit(tarantool_atexit);
> 
> This will allow us to extend tarantool_atexit()
> with other cleanup routines if we need to.
> Sounds reasonable?

Well, we can append other routines to tarantool_atexit anyway.
I wrapped the code into free_rl_state() in the previous patch, but
it was inlined into tarantool_free «as_is» previously.
I mean

tarantool_free(void)
{
	/* Other cleanup routines */
	if (isatty(…)) {
		/* Clean rl state. */
	}
	/* Other cleanup routines */
}

But no problem, here’s the incremental diff
 
diff --git a/src/main.cc b/src/main.cc
index a1572b81e..e674d85b1 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -589,15 +589,8 @@ load_cfg()
 }
 
 void
-tarantool_atexit(void)
+free_rl_state(void)
 {
-       /* Same checks as in tarantool_free() */
-       if (getpid() != master_pid)
-               return;
-
-       if (!cord_is_main())
-               return;
-
        /* tarantool_lua_free() was formerly reponsible for terminal reset,
         * but it is no longer called
         */
@@ -610,6 +603,19 @@ tarantool_atexit(void)
        }
 }
 
+void
+tarantool_atexit(void)
+{
+       /* Same checks as in tarantool_free() */
+       if (getpid() != master_pid)
+               return;
+
+       if (!cord_is_main())
+               return;
+
+       free_rl_state();
+}
+
 void
 tarantool_free(void)
 {


--
Serge Petrenko
sergepetrenko@tarantool.org

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Cyrill Gorcunov]

On Fri, Nov 29, 2019 at 03:20:16PM +0300, Serge Petrenko wrote:
...
> But no problem, here’s the incremental diff
>  
> diff --git a/src/main.cc b/src/main.cc
> index a1572b81e..e674d85b1 100644
>  
> +void
> +tarantool_atexit(void)
> +{
> +       /* Same checks as in tarantool_free() */
> +       if (getpid() != master_pid)
> +               return;
> +
> +       if (!cord_is_main())
> +               return;
> +
> +       free_rl_state();
> +}

As to me this is a way cleaner. Ack from my side.
Lets wait for Kostya's opinion.

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/11/29 16:31]:
> > +void
> > +tarantool_atexit(void)
> > +{
> > +       /* Same checks as in tarantool_free() */
> > +       if (getpid() != master_pid)
> > +               return;
> > +
> > +       if (!cord_is_main())
> > +               return;
> > +
> > +       free_rl_state();
> > +}
> 
> As to me this is a way cleaner. Ack from my side.
> Lets wait for Kostya's opinion.

I'm happy with it, since it has a comment :)

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Kirill Yukhin]

Hello,

On 26 ноя 18:15, Serge Petrenko wrote:
> The tarantool_free() call in the end of main() works all the time except
> when we exit due to a panic. We need to clear terminal state in this
> case also, so return to using atexit() to clear readline state.
> 
> Closes #4466
> ---
> https://github.com/tarantool/tarantool/issues/4466
> https://github.com/tarantool/tarantool/tree/sp/gh-4466-clear-rl-state
> 
>  src/main.cc | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)

I've checked your patch into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Alexander Turenko]

On Tue, Dec 10, 2019 at 05:03:11PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 26 ноя 18:15, Serge Petrenko wrote:
> > The tarantool_free() call in the end of main() works all the time except
> > when we exit due to a panic. We need to clear terminal state in this
> > case also, so return to using atexit() to clear readline state.
> > 
> > Closes #4466
> > ---
> > https://github.com/tarantool/tarantool/issues/4466
> > https://github.com/tarantool/tarantool/tree/sp/gh-4466-clear-rl-state
> > 
> >  src/main.cc | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)

I'm okay with the change itself.

> I've checked your patch into master.

Kirill, please, propagate into all supported long-term branches: this is
the bugfix.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] clear terminal state on panic

[Alexander Turenko]

On Mon, Dec 23, 2019 at 05:29:31PM +0300, Alexander Turenko wrote:
> On Tue, Dec 10, 2019 at 05:03:11PM +0300, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 26 ноя 18:15, Serge Petrenko wrote:
> > > The tarantool_free() call in the end of main() works all the time except
> > > when we exit due to a panic. We need to clear terminal state in this
> > > case also, so return to using atexit() to clear readline state.
> > > 
> > > Closes #4466
> > > ---
> > > https://github.com/tarantool/tarantool/issues/4466
> > > https://github.com/tarantool/tarantool/tree/sp/gh-4466-clear-rl-state
> > > 
> > >  src/main.cc | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> I'm okay with the change itself.
> 
> > I've checked your patch into master.
> 
> Kirill, please, propagate into all supported long-term branches: this is
> the bugfix.

Kirill approved the cherry-pick. The patch is already in 2.3 and master.
I pushed it to 2.2 and 1.10:

1.10.4-81-gbac732eef
2.2.2-5-g821cc167e

WBR, Alexander Turenko.