| Message ID | 20240617104326.3522535-3-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cat-file related doc and test | expand |
Eric Wong <e@80x24.org> writes: > Buffering by default breaks some 3rd-party Perl scripts using > cat-file, but this breakage was not detected anywhere in our > test suite. The easiest place to test this behavior is with > Git.pm, since (AFAIK) other equivalent way to test this behavior > from Bourne shell and/or awk would require racy sleeps, > non-portable FIFOs or tedious C code. Yes, using Perl is a good substitute for writing it in C in this case. I however question the choice to use t9700/test.pl here, which is clearly stated that its purpose is to "test perl interface which is Git.pm", and added tests are not testing anything in Git.pm at all. Using t9700/test.pl only because it happens to use "perl -MTest::More" sounds a bit eh, suboptimal. It seems that there are Perl snippets in other tests (including t1006 that is specifically about cat-file). How involved would it be to implement these new tests without modifying unrelated test scripts? > Signed-off-by: Eric Wong <e@80x24.org> > --- > t/t9700/test.pl | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t9700/test.pl b/t/t9700/test.pl > index d8e85482ab..94a2e2c09d 100755 > --- a/t/t9700/test.pl > +++ b/t/t9700/test.pl > @@ -154,6 +154,20 @@ sub adjust_dirsep { > "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ", > 'unquote escape sequences'); > > +# ensure --batch-check is unbuffered by default > +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check)); > +print $out $file1hash, "\n" or die $!; > +my $info = <$in>; > +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check'; > +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); > + > +# ditto with `info' with --batch-command > +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command)); > +print $out 'info ', $file1hash, "\n" or die $!; > +$info = <$in>; > +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info'; > +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); > + > printf "1..%d\n", Test::More->builder->current_test; > > my $is_passing = eval { Test::More->is_passing };
Eric Wong <e@80x24.org> writes: > While working on buffering changes to `git cat-file' in a > separate patch, I inadvertently made the output of --batch-check > and the `info' command of --batch-command buffered by default. Here "buffered" means "as if opt->buffer_output is turned on", which in turn means "the output goes through stdio"? Just making sure that my understanding of what the breakage was is in line with what you wanted to convey. Thanks.
Hi Eric On 17/06/2024 11:43, Eric Wong wrote: > +# ensure --batch-check is unbuffered by default > +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check)); > +print $out $file1hash, "\n" or die $!; It's been a while since I did any perl scripting and I'm not clear whether $out is buffered or not and if it is whether it is guaranteed to be flushed when we print "\n". It might be worth adding a explicit flush so it is clear that any deadlocks come from cat-file and not our test code. > +my $info = <$in>; Is there an easy way to add a timeout to this read so that the failure mode isn't "the test hangs without printing anything"? I'm not sure that failure mode is easy to diagnose from our CI output as it is hard to tell which test caused the CI to timeout and it takes ages for the CI to time out. Best Wishes Phillip > +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check'; > +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); > + > +# ditto with `info' with --batch-command > +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command)); > +print $out 'info ', $file1hash, "\n" or die $!; > +$info = <$in>; > +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info'; > +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); > + > printf "1..%d\n", Test::More->builder->current_test; > > my $is_passing = eval { Test::More->is_passing }; >
Phillip Wood <phillip.wood123@gmail.com> wrote: > Hi Eric > > On 17/06/2024 11:43, Eric Wong wrote: > > +# ensure --batch-check is unbuffered by default > > +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check)); > > +print $out $file1hash, "\n" or die $!; > > It's been a while since I did any perl scripting and I'm not clear whether > $out is buffered or not and if it is whether it is guaranteed to be flushed > when we print "\n". It might be worth adding a explicit flush so it is clear > that any deadlocks come from cat-file and not our test code. Pipes and sockets created by Perl are always unbuffered since 5.8, at least. If they were buffered, Git.pm users (including git-svn) wouldn't have worked at all. > > +my $info = <$in>; > > Is there an easy way to add a timeout to this read so that the failure mode > isn't "the test hangs without printing anything"? I'm not sure that failure > mode is easy to diagnose from our CI output as it is hard to tell which test > caused the CI to timeout and it takes ages for the CI to time out. Yeah, select() has been added in v2.
On 19/06/2024 19:08, Eric Wong wrote: > Phillip Wood <phillip.wood123@gmail.com> wrote: >> Hi Eric >> >> On 17/06/2024 11:43, Eric Wong wrote: >>> +# ensure --batch-check is unbuffered by default >>> +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check)); >>> +print $out $file1hash, "\n" or die $!; >> >> It's been a while since I did any perl scripting and I'm not clear whether >> $out is buffered or not and if it is whether it is guaranteed to be flushed >> when we print "\n". It might be worth adding a explicit flush so it is clear >> that any deadlocks come from cat-file and not our test code. > > Pipes and sockets created by Perl are always unbuffered since > 5.8, at least. If they were buffered, Git.pm users (including > git-svn) wouldn't have worked at all. Thanks for clarifying that >>> +my $info = <$in>; >> >> Is there an easy way to add a timeout to this read so that the failure mode >> isn't "the test hangs without printing anything"? I'm not sure that failure >> mode is easy to diagnose from our CI output as it is hard to tell which test >> caused the CI to timeout and it takes ages for the CI to time out. > > Yeah, select() has been added in v2. That's much nicer. Thanks Phillip
diff --git a/t/t9700/test.pl b/t/t9700/test.pl index d8e85482ab..94a2e2c09d 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -154,6 +154,20 @@ sub adjust_dirsep { "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ", 'unquote escape sequences'); +# ensure --batch-check is unbuffered by default +my ($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-check)); +print $out $file1hash, "\n" or die $!; +my $info = <$in>; +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-check'; +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); + +# ditto with `info' with --batch-command +($pid, $in, $out, $ctx) = $r->command_bidi_pipe(qw(cat-file --batch-command)); +print $out 'info ', $file1hash, "\n" or die $!; +$info = <$in>; +is $info, "$file1hash blob 15\n", 'command_bidi_pipe w/ --batch-command=info'; +$r->command_close_bidi_pipe($pid, $in, $out, $ctx); + printf "1..%d\n", Test::More->builder->current_test; my $is_passing = eval { Test::More->is_passing };
While working on buffering changes to `git cat-file' in a separate patch, I inadvertently made the output of --batch-check and the `info' command of --batch-command buffered by default. Buffering by default breaks some 3rd-party Perl scripts using cat-file, but this breakage was not detected anywhere in our test suite. The easiest place to test this behavior is with Git.pm, since (AFAIK) other equivalent way to test this behavior from Bourne shell and/or awk would require racy sleeps, non-portable FIFOs or tedious C code. Signed-off-by: Eric Wong <e@80x24.org> --- t/t9700/test.pl | 14 ++++++++++++++ 1 file changed, 14 insertions(+)