Re: [PATCH v2 0/3] am: let command-line options override saved options

From: Paul Tan <pyokagan@gmail.com>
Date: Aug 5, 2015, 17:51 UTC
Message-ID: <20150805175117.GA6759@yoshi.chippynet.com>
In-Reply-To: <xmqq37zx68uf.fsf@gitster.dls.corp.google.com>

On Wed, Aug 05, 2015 at 08:41:44AM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> Interesting.  This seems to break test under prove.
> 
>     cd t && make T=t4153-am-resume-override-opts.sh prove
> 
> does not seem to return.

The new test-terminal.perl code is the culprit. It seems that if our wrapped process terminates before our stdin-writing fork does, our stdin-writing process will stall. I think this occurs with prove because prove waits until all of its child processes terminate before returning.

So, the solution may be to send a SIGTERM to our stdin-writing fork should our wrapped process terminate before it does, in order to ensure that it immediately exits.

The following squash fixes it for me.

Thanks, Paul

-- >8 --
Subject: [PATCH] squash! test_terminal: redirect child process' stdin to a pty

When the child process terminates before the copy_stdio() finishes writing all of its data to the child's stdin slave pty, it will stall.

As such, we first move the stdin-pty-writing logic out of copy_stdio() into its own subroutine copy_stdin() so that we can manage the forked process ourselves, and then we send SIGTERM to the forked process should the command we are wrapping terminate before copy_stdin() finishes writing all of its data to un-stall it.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/test-terminal.perl | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index f6fc9ae..96b6a03 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -51,24 +51,26 @@ sub xsendfile {
 	copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
 }
 
-sub copy_stdio {
-	my ($in, $out, $err) = @_;
+sub copy_stdin {
+	my ($in) = @_;
 	my $pid = fork;
 	if (!$pid) {
-		close($out);
-		close($err);
 		xsendfile($in, \*STDIN);
 		exit 0;
 	}
-	$pid = fork;
+	close($in);
+	return $pid;
+}
+
+sub copy_stdio {
+	my ($out, $err) = @_;
+	my $pid = fork;
 	defined $pid or die "fork failed: $!";
 	if (!$pid) {
-		close($in);
 		close($out);
 		xsendfile(\*STDERR, $err);
 		exit 0;
 	}
-	close($in);
 	close($err);
 	xsendfile(\*STDOUT, $out);
 	finish_child($pid) == 0
@@ -91,5 +93,12 @@ my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err
 close $master_in->slave;
 close $master_out->slave;
 close $master_err->slave;
-copy_stdio($master_in, $master_out, $master_err);
-exit(finish_child($pid));
+my $in_pid = copy_stdin($master_in);
+copy_stdio($master_out, $master_err);
+my $ret = finish_child($pid);
+# If the child process terminates before our copy_stdin() process is able to
+# write all of its data to $master_in, the copy_stdin() process could stall.
+# Send SIGTERM to it to ensure it terminates.
+kill 'TERM', $in_pid;
+finish_child($in_pid);
+exit($ret);

-- 
2.5.0.282.gdd6b4b0