Merge lp:~niclasl/pantheon-terminal/fix-1327526 into lp:~elementary-apps/pantheon-terminal/trunk

[Niclas Lockner]

Fixed shutdown when the user cancels the dialog as described in #1327526

1. Wait with the shell termination until all dialog windows has been processed and it is known that the window will close

2. Retry to terminate the shell as long as it hasn't terminated (Fixes an issue where the shell didn't terminate when kill_fg_and_term_ps was called)

[lp:~niclasl/pantheon-terminal/fix-1327526]

592. By Niclas Lockner on 2014-08-05

Avoid a possible endless loop when terminating the shell

[Raphael Isemann (teemperor)]

Fixes said bug and the rest still works. Code-style is correct too.

The thing that bugs me is the loop at the end:

1.
Posix.usleep (100);
is also available in the GLib via
GLib.Thread.usleep (100);

2. I those lines should to be the other way round, as we first should send a signal and then wait.

Posix.usleep (100);
Posix.kill (this.child_pid, Posix.SIGTERM);

3. This
Posix.kill (this.child_pid, 0) != -1
can be
Posix.kill (this.child_pid, 0) == 0
on both occurrences.

Also a comment like
// Check if PID is valid
should be added as it's not that obvious what kill with sig 0 does.

4. I think we shouldn't SIGKILL here, but not sure what we should do instead. Let's add shnatsel and see what his opinion is on this topic.

Posix.kill (this.child_pid, Posix.SIGKILL);

review: Needs Fixing (code and functionality)

[Sergey "Shnatsel" Davidoff (shnatsel)]

Do not sigkill without a good reason! An illustration of proper use of sigkill can be seen in window managers for unresponsive applications, I believe you should just mirror that.

[lp:~niclasl/pantheon-terminal/fix-1327526]

593. By Niclas Lockner on 2014-08-05

Revert to not kill the shell

594. By Niclas Lockner on 2014-08-05

Remove unused variable

[Raphael Isemann (teemperor)]

Oh, line 46 in the diff, the first

Posix.kill (this.child_pid, Posix.SIGTERM);

is unecessary.

review: Needs Fixing

[lp:~niclasl/pantheon-terminal/fix-1327526]

595. By Niclas Lockner on 2014-08-05

Remove redundant kill call

[Sergey "Shnatsel" Davidoff (shnatsel)]

Fine with me for now. Just document the potential issue with infinite loop in the end in a new bug report and go ahead. Thanks!

review: Approve

[Raphael Isemann (teemperor)]

Ok, i'm fine with it too.

The problem is now in a extra-bugreport: https://bugs.launchpad.net/pantheon-terminal/+bug/1353159

review: Approve (code and functionality)

=== modified file 'src/PantheonTerminalWindow.vala'
--- src/PantheonTerminalWindow.vala	2014-08-01 21:31:33 +0000
+++ src/PantheonTerminalWindow.vala	2014-08-05 20:58:53 +0000
@@ -524,6 +524,7 @@
             update_saved_window_state ();
             action_quit ();
             string tabs = "";
+            var tabs_to_terminate = new GLib.List <TerminalWidget> ();
 
             foreach (var t in terminals) {
                 t = (TerminalWidget) t;
@@ -531,18 +532,20 @@
                 if (t.has_foreground_process ()) {
                     var d = new ForegroundProcessDialog.before_close ();
                     if (d.run () == 1) {
-                        t.kill_fg_and_term_ps ();
+                        t.kill_fg ();
                         d.destroy ();
                     } else {
                         d.destroy ();
                         return true;
                     }
-
-                } else {
-                    t.term_ps ();
                 }
+
+                tabs_to_terminate.append (t);
             }
 
+            foreach (var t in tabs_to_terminate)
+                t.term_ps ();
+
             saved_state.tabs = tabs;
             return false;
         }

=== modified file 'src/TerminalWidget.vala'
--- src/TerminalWidget.vala	2014-08-05 18:12:39 +0000
+++ src/TerminalWidget.vala	2014-08-05 20:58:53 +0000
@@ -242,13 +242,12 @@
 
         public void term_ps () {
             killed = true;
-            //this.pty_object.close ();
-            Posix.kill (this.child_pid, Posix.SIGTERM);
-        }
 
-        public void kill_fg_and_term_ps () {
-            kill_fg ();
-            term_ps ();
+            /* Check if the shell process is still alive by sending 0 signals */
+            while (Posix.kill (this.child_pid, 0) == 0) {
+                Posix.kill (this.child_pid, Posix.SIGTERM);
+                Thread.usleep (100);
+            }
         }
 
         public void active_shell (string dir = GLib.Environment.get_current_dir ()) {