| Message ID | d95180fc-8f8a-4e1d-987d-3aa0811be7de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Hi Rubén On 03/06/2024 21:38, Rubén Justo wrote: > In 18d8c26930 (test_terminal: redirect child process' stdin to a pty, > 2015-08-04), t/test-terminal.perl learned to connect the child process' > stdin to a pty. It works well for what was intended: satisfying an > `isatty(STDIN_FILENO)` check. > > However, the fork introduced, that copies the stdin to the child > process, does not always manage to send all the information. I think the problem maybe to do with the use of File::Copy, not with the fork. The man page for the copy function says Note that passing in files as handles instead of names may lead to loss of information on some operating systems; it is recommended that you use file names whenever possible. Rather than adding a new flag to work around a bug in our script it might be better to try and fix the bug by using a loop that reads blocks of data from the source and writes them to the destination instead of calling copy. Best Wishes Phillip > To illustrate this behaviour, we can use a function like this: > > f () > { > dd if=/dev/zero bs=1 count=10000 status=none | > t/test-terminal.perl cat - 2>/dev/null | > wc -c; > } > > We do not obtain the expected results when executing this function > 100 times: > > $ for i in $(seq 100); do f; done | sort | uniq -c > 36 0 > 4 1 > 53 4095 > 7 4159 > > If we do the same with a version that does not redirect stdin, a version > prior to 18d8c26930, the expected result is obtained: > > $ git checkout 18d8c26930~1 > $ for i in $(seq 100); do f; done | sort | uniq -c > 100 10000 > > In a subsequent commit, a new test is going to rely on test-terminate, > and it does not require stdin to be connected to a terminal, but all > piped data needs to be successfully transmitted to the child process. > > To make this possible, add a new parameter "--no-stdin-pty" to allow > disabling the stdin redirection though a pty. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > t/test-terminal.perl | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/t/test-terminal.perl b/t/test-terminal.perl > index 3810e9bb43..85edc9e8b9 100755 > --- a/t/test-terminal.perl > +++ b/t/test-terminal.perl > @@ -12,10 +12,10 @@ sub start_child { > if (not defined $pid) { > die "fork failed: $!" > } elsif ($pid == 0) { > - open STDIN, "<&", $in; > + open STDIN, "<&", $in if $in; > open STDOUT, ">&", $out; > open STDERR, ">&", $err; > - close $in; > + close $in if $in; > close $out; > exec(@$argv) or die "cannot exec '$argv->[0]': $!" > } > @@ -78,28 +78,32 @@ sub copy_stdio { > } > > if ($#ARGV < 1) { > - die "usage: test-terminal program args"; > + die "usage: test-terminal [--no-stdin-pty] program args"; > } > +my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty'; > +shift @ARGV if $no_stdin_pty; > $ENV{TERM} = 'vt100'; > -my $parent_in = new IO::Pty; > +my $parent_in = $no_stdin_pty ? undef : IO::Pty->new; > my $parent_out = new IO::Pty; > my $parent_err = new IO::Pty; > -$parent_in->set_raw(); > +$parent_in->set_raw() if $parent_in; > $parent_out->set_raw(); > $parent_err->set_raw(); > -$parent_in->slave->set_raw(); > +$parent_in->slave->set_raw() if $parent_in; > $parent_out->slave->set_raw(); > $parent_err->slave->set_raw(); > -my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave); > -close $parent_in->slave; > +my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave); > +close $parent_in->slave if $parent_in; > close $parent_out->slave; > close $parent_err->slave; > -my $in_pid = copy_stdin($parent_in); > +my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in); > copy_stdio($parent_out, $parent_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 $parent_in, the copy_stdin() process could stall. > -# Send SIGTERM to it to ensure it terminates. > -kill 'TERM', $in_pid; > -finish_child($in_pid); > +if ($in_pid) { > + # If the child process terminates before our copy_stdin() process is able to > + # write all of its data to $parent_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);
On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote: > Hi Rubén > > On 03/06/2024 21:38, Rubén Justo wrote: > > In 18d8c26930 (test_terminal: redirect child process' stdin to a pty, > > 2015-08-04), t/test-terminal.perl learned to connect the child process' > > stdin to a pty. It works well for what was intended: satisfying an > > `isatty(STDIN_FILENO)` check. > > > > However, the fork introduced, that copies the stdin to the child > > process, does not always manage to send all the information. > > I think the problem maybe to do with the use of File::Copy, not with the > fork. The man page for the copy function says > > Note that passing in files as handles instead of names may > lead to loss of information on some operating systems; it is > recommended that you use file names whenever possible. > > Rather than adding a new flag to work around a bug in our script it might be > better to try and fix the bug by using a loop that reads blocks of data from > the source and writes them to the destination instead of calling copy. I don't think I've seen missing data. But do note that the test_terminal stdin handling is racy: https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/ IMHO we should consider getting rid of it entirely. I think the only thing that uses it is t4153 (AFAICT it is luckily not racy because it does not actually read stdin, but only checks isatty). -Peff
Jeff King <peff@peff.net> writes: > IMHO we should consider getting rid of it entirely. I think the only > thing that uses it is t4153 (AFAICT it is luckily not racy because it > does not actually read stdin, but only checks isatty). Sounds like a better approach than piling another workaround on the test helper. Reading the old discussion, we seem to have been in agreement that we generally do not have to insist reading from a tty and certainly the "add -p" codepath is not one of those "if your other payload must come from the standard input, your instructions to specify how to handle that data needs to come from elsewhere, and that is /dev/tty" cases. Thanks.
On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote: > Rather than adding a new flag to work around a bug in our script it might be > better to try and fix the bug by using a loop that reads blocks of data from > the source and writes them to the destination instead of calling copy. To be honest, I've tried. I haven't found a way to fix it properly. I also thought about removing it. I agree with Peff, removing the stdin redirection makes more sense. IMHO simplifies. But, perhaps we can happily live another almost-decade with this new --no-stdin-pty, before finally removing the stdin redirection in this helper. :-)
On Wed, Jun 05, 2024 at 08:39:59AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > IMHO we should consider getting rid of it entirely. I think the only > > thing that uses it is t4153 (AFAICT it is luckily not racy because it > > does not actually read stdin, but only checks isatty). > > Sounds like a better approach than piling another workaround on the > test helper. Reading the old discussion, we seem to have been in > agreement that we generally do not have to insist reading from a tty > and certainly the "add -p" codepath is not one of those "if your > other payload must come from the standard input, your instructions > to specify how to handle that data needs to come from elsewhere, and > that is /dev/tty" cases. I think we got rid of all of the "read interactive input over tty" cases. The one that I still see is am's heuristic to use isatty() to decide whether the user might send patches over stdin. I just sent a separate patch series which I think improves the situation. And then either Rubén could build on top of that (if we think it will graduate quickly) or he could do his optional patch, and we could rip it back out when the two are merged. -Peff
On Thu, Jun 06, 2024 at 12:50:39AM +0200, Rubén Justo wrote: > On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote: > > > Rather than adding a new flag to work around a bug in our script it might be > > better to try and fix the bug by using a loop that reads blocks of data from > > the source and writes them to the destination instead of calling copy. > > To be honest, I've tried. I haven't found a way to fix it properly. I think File::Copy() is not the culprit. The problem is more at the syscall level. Once test_terminal finishes writing all of the output to the tty, it closes its end of the descriptor. And now the tty is "gone", isatty() returns false, and further reads get EIO (even if we didn't read all of the bytes! They're lost). So I think the only thing we could do is _not_ actually close the descriptor. But now we don't have a way to signal EOF to the other side. Unless perhaps there is some clever way to do so. I'd still favor ripping it out. > I also thought about removing it. I agree with Peff, removing the stdin > redirection makes more sense. IMHO simplifies. > > But, perhaps we can happily live another almost-decade with this new > --no-stdin-pty, before finally removing the stdin redirection in this > helper. :-) Hopefully not a decade. ;) I just sent a series, and I think you could either build on top, or the merge resolution could just drop your new option. -Peff
On Thu, Jun 06, 2024 at 04:27:48AM -0400, Jeff King wrote: > I just sent a series, and I think you could either build on top, or > the merge resolution could just drop your new option. I've already responded to your series, but just to confirm here. I'm glad to drop this step and building on top of jk/am-retry. Thanks!
diff --git a/t/test-terminal.perl b/t/test-terminal.perl index 3810e9bb43..85edc9e8b9 100755 --- a/t/test-terminal.perl +++ b/t/test-terminal.perl @@ -12,10 +12,10 @@ sub start_child { if (not defined $pid) { die "fork failed: $!" } elsif ($pid == 0) { - open STDIN, "<&", $in; + open STDIN, "<&", $in if $in; open STDOUT, ">&", $out; open STDERR, ">&", $err; - close $in; + close $in if $in; close $out; exec(@$argv) or die "cannot exec '$argv->[0]': $!" } @@ -78,28 +78,32 @@ sub copy_stdio { } if ($#ARGV < 1) { - die "usage: test-terminal program args"; + die "usage: test-terminal [--no-stdin-pty] program args"; } +my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty'; +shift @ARGV if $no_stdin_pty; $ENV{TERM} = 'vt100'; -my $parent_in = new IO::Pty; +my $parent_in = $no_stdin_pty ? undef : IO::Pty->new; my $parent_out = new IO::Pty; my $parent_err = new IO::Pty; -$parent_in->set_raw(); +$parent_in->set_raw() if $parent_in; $parent_out->set_raw(); $parent_err->set_raw(); -$parent_in->slave->set_raw(); +$parent_in->slave->set_raw() if $parent_in; $parent_out->slave->set_raw(); $parent_err->slave->set_raw(); -my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave); -close $parent_in->slave; +my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave); +close $parent_in->slave if $parent_in; close $parent_out->slave; close $parent_err->slave; -my $in_pid = copy_stdin($parent_in); +my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in); copy_stdio($parent_out, $parent_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 $parent_in, the copy_stdin() process could stall. -# Send SIGTERM to it to ensure it terminates. -kill 'TERM', $in_pid; -finish_child($in_pid); +if ($in_pid) { + # If the child process terminates before our copy_stdin() process is able to + # write all of its data to $parent_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);
In 18d8c26930 (test_terminal: redirect child process' stdin to a pty, 2015-08-04), t/test-terminal.perl learned to connect the child process' stdin to a pty. It works well for what was intended: satisfying an `isatty(STDIN_FILENO)` check. However, the fork introduced, that copies the stdin to the child process, does not always manage to send all the information. To illustrate this behaviour, we can use a function like this: f () { dd if=/dev/zero bs=1 count=10000 status=none | t/test-terminal.perl cat - 2>/dev/null | wc -c; } We do not obtain the expected results when executing this function 100 times: $ for i in $(seq 100); do f; done | sort | uniq -c 36 0 4 1 53 4095 7 4159 If we do the same with a version that does not redirect stdin, a version prior to 18d8c26930, the expected result is obtained: $ git checkout 18d8c26930~1 $ for i in $(seq 100); do f; done | sort | uniq -c 100 10000 In a subsequent commit, a new test is going to rely on test-terminate, and it does not require stdin to be connected to a terminal, but all piped data needs to be successfully transmitted to the child process. To make this possible, add a new parameter "--no-stdin-pty" to allow disabling the stdin redirection though a pty. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- t/test-terminal.perl | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)