Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] t/io/closepid.t hangs for test timeout (50 seconds) under harness #13535

Closed
p5pRT opened this issue Jan 17, 2014 · 24 comments
Closed

[PATCH] t/io/closepid.t hangs for test timeout (50 seconds) under harness #13535

p5pRT opened this issue Jan 17, 2014 · 24 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jan 17, 2014

Migrated from rt.perl.org#121028 (status was 'resolved')

Searchable as RT121028$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2014

From @wolfsage

Created by wolfsage@gmail.com

$ time ./perl harness io/closepid.t

  io/closepid.t .. ok
  All tests successful.
  Files=1, Tests=3, 50 wallclock secs ( 0.00 usr + 0.01 sys = 0.01 CPU)
  Result​: PASS

  real 0m50.079s
  user 0m0.056s
  sys 0m0.020s

With patch​:

  $ time ./perl harness io/closepid.t
  io/closepid.t .. ok
  All tests successful.
  Files=1, Tests=3, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02
cusr 0.00 csys = 0.03 CPU)
  Result​: PASS

  real 0m0.093s
  user 0m0.080s
  sys 0m0.012s

The problem seems to be that the test is forking a perl process like so​:

  my $pid = open my $fh1, qq/$perl -e "sleep 50" |/;

Which starts two processes, 'sh -c "$perl -e \"sleep 50\""' and the perl -e...

The pid from sh is returned, and later we try to clean up by sending it a HUP​:

  kill $killsig, $pid;

On my system, sh -c ".." doesn't exit on a HUP, but it does on a TERM.

I've updated the test to use TERM instead, however I'm not sure if this
violates the original test case.

Tony, it looks like you added this in e9d373c, can you comment?

Thanks,

-- Matthew Horsfall (alh)

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.14.2:

Configured by Debian Project at Mon Mar 18 19:16:26 UTC 2013.

Summary of my perl5 (revision 5 version 14 subversion 2) configuration:

  Platform:
    osname=linux, osvers=2.6.42-37-generic,
archname=x86_64-linux-gnu-thread-multi
    uname='linux batsu 2.6.42-37-generic #58-ubuntu smp thu jan 24
15:28:10 utc 2013 x86_64 x86_64 x86_64 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN
-Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr
-Dprivlib=/usr/share/perl/5.14 -Darchlib=/usr/lib/perl/5.14
-Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5
-Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl/5.14.2
-Dsitearch=/usr/local/lib/perl/5.14.2 -Dman1dir=/usr/share/man/man1
-Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1
-Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1
-Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh
-Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -DDEBUGGING=-g -Doptimize=-O2
-Duseshrplib -Dlibperl=libperl.so.5.14.2 -des'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.6.3', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=, so=so, useshrplib=true, libperl=libperl.so.5.14.2
    gnulibc_version='2.15'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib
-fstack-protector'

Locally applied patches:



@INC for perl 5.14.2:
    /etc/perl
    /usr/local/lib/perl/5.14.2
    /usr/local/share/perl/5.14.2
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.14
    /usr/share/perl/5.14
    /usr/local/lib/site_perl
    .


Environment for perl 5.14.2:
    HOME=/home/mhorsfall
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/mhorsfall/perl5/perlbrew/bin:/home/mhorsfall/bin:/home/mhorsfall/bin:/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
    PERLBREW_BASHRC_VERSION=0.66
    PERLBREW_HOME=/home/mhorsfall/.perlbrew
    PERLBREW_ROOT=/home/mhorsfall/perl5/perlbrew
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2014

From @wolfsage

0001-Switch-from-HUP-to-TERM-so-test-sub-process-exits-wh.patch
From 0483c09e8bfccff1b4255f178be5fd012bc0da11 Mon Sep 17 00:00:00 2001
From: Matthew Horsfall <WolfSage@gmail.com>
Date: Fri, 17 Jan 2014 18:33:08 -0500
Subject: [PATCH] Switch from HUP to TERM so test sub-process exits when asked
 for under harness.

  'make test' does not show the slow behaviour (t/TEST), only under
  TAP::Harness (t/harness) do we see it.
---
 t/io/closepid.t |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/io/closepid.t b/t/io/closepid.t
index aa937f5..bdd8c8e 100644
--- a/t/io/closepid.t
+++ b/t/io/closepid.t
@@ -16,13 +16,13 @@ watchdog(10, $^O eq 'MSWin32' ? "alarm" : '');
 use Config;
 $| = 1;
 $SIG{PIPE} = 'IGNORE';
-$SIG{HUP} = 'IGNORE' if $^O eq 'interix';
+$SIG{TERM} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
 $perl .= qq[ "-I../lib"];
 
-my $killsig = 'HUP';
-$killsig = 1 unless $Config{sig_name} =~ /\bHUP\b/;
+my $killsig = 'TERM';
+$killsig = 15 unless $Config{sig_name} =~ /\bTERM\b/;
 
 SKIP:
 {
-- 
1.7.9.5

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 19, 2014

From @tonycoz

On Fri Jan 17 15​:36​:14 2014, alh wrote​:

$ time ./perl harness io/closepid.t

io/closepid.t .. ok
All tests successful.
Files=1, Tests=3, 50 wallclock secs ( 0.00 usr + 0.01 sys = 0.01
CPU)
Result​: PASS

real 0m50.079s
user 0m0.056s
sys 0m0.020s

With patch​:

$ time ./perl harness io/closepid.t
io/closepid.t .. ok
All tests successful.
Files=1, Tests=3, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02
cusr 0.00 csys = 0.03 CPU)
Result​: PASS

real 0m0.093s
user 0m0.080s
sys 0m0.012s

The problem seems to be that the test is forking a perl process like
so​:

my $pid = open my $fh1, qq/$perl -e "sleep 50" |/;

Which starts two processes, 'sh -c "$perl -e \"sleep 50\""' and the
perl -e...

The pid from sh is returned, and later we try to clean up by sending
it a HUP​:

kill $killsig, $pid;

On my system, sh -c ".." doesn't exit on a HUP, but it does on a TERM.

I've updated the test to use TERM instead, however I'm not sure if
this
violates the original test case.

Tony, it looks like you added this in e9d373c, can you comment?

The change in signal should make no difference to what test is trying to test for and I can reproduce the delay you see.

Your change doesn't make any difference to the delay for me though​:

# blead is 3ac2191
# closepid-delay is blead plus your patch
tony@​mars​:.../perl2/t$ git checkout blead
Switched to branch 'blead'
tony@​mars​:.../perl2/t$ time ./perl harness io/closepid.t
io/closepid.t .. ok
All tests successful.
Files=1, Tests=3, 50 wallclock secs ( 0.01 usr 0.00 sys + 0.01 cusr 0.01 csys = 0.03 CPU)
Result​: PASS

real 0m50.124s
user 0m0.112s
sys 0m0.016s
tony@​mars​:.../perl2/t$ git checkout closepid-delay
Switched to branch 'closepid-delay'
tony@​mars​:.../perl2/t$ git show
commit 5077467e313c7cff7a4b951cb49c744f4a0e3125
Author​: Matthew Horsfall <WolfSage@​gmail.com>
Date​: Fri Jan 17 18​:33​:08 2014 -0500

  Switch from HUP to TERM so test sub-process exits when asked for under harne
 
  'make test' does not show the slow behaviour (t/TEST), only under
  TAP​::Harness (t/harness) do we see it.

Inline Patch
diff --git a/t/io/closepid.t b/t/io/closepid.t
index aa937f5..bdd8c8e 100644
--- a/t/io/closepid.t
+++ b/t/io/closepid.t
@@ -16,13 +16,13 @@ watchdog(10, $^O eq 'MSWin32' ? "alarm" : '');
 use Config;
 $| = 1;
 $SIG{PIPE} = 'IGNORE';
-$SIG{HUP} = 'IGNORE' if $^O eq 'interix';
+$SIG{TERM} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
 $perl .= qq[ "-I../lib"];
 
-my $killsig = 'HUP';
-$killsig = 1 unless $Config{sig_name} =~ /\bHUP\b/;
+my $killsig = 'TERM';
+$killsig = 15 unless $Config{sig_name} =~ /\bTERM\b/;
 
 SKIP:
 {
tony@mars:.../perl2/t$ time ./perl harness io/closepid.t io/closepid\.t \.\. ok All tests successful\. Files=1\, Tests=3\, 50 wallclock secs \( 0\.01 usr 0\.00 sys \+ 0\.02 cusr 0\.00 csys = 0\.03 CPU\) Result​: PASS

real 0m50.108s
user 0m0.100s
sys 0m0.008s

A different approach is to not use the shell, please let me know if the attached fixes the problem for you.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 19, 2014

From @tonycoz

0001-perl-121028-avoid-creating-a-shell-process.patch
From 4b0a73e22aa3e59e48b32ef2409ea89ed5c3f125 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 20 Jan 2014 09:39:19 +1100
Subject: [perl #121028] avoid creating a shell process

---
 t/io/closepid.t |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/io/closepid.t b/t/io/closepid.t
index aa937f5..b9e9e15 100644
--- a/t/io/closepid.t
+++ b/t/io/closepid.t
@@ -19,7 +19,6 @@ $SIG{PIPE} = 'IGNORE';
 $SIG{HUP} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
-$perl .= qq[ "-I../lib"];
 
 my $killsig = 'HUP';
 $killsig = 1 unless $Config{sig_name} =~ /\bHUP\b/;
@@ -34,7 +33,7 @@ SKIP:
     # close on the original of a popen handle dupped to a standard handle
     # would wait4pid(0, ...)
     open my $savein, "<&", \*STDIN;
-    my $pid = open my $fh1, qq/$perl -e "sleep 50" |/;
+    my $pid = open my $fh1, "-|", $perl, "-e", "sleep 50";
     ok($pid, "open a pipe");
     # at this point PL_fdpids[fileno($fh1)] is the pid of the new process
     ok(open(STDIN, "<&=", $fh1), "dup the pipe");
-- 
1.7.10.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 19, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 20, 2014

From @wolfsage

On Sun, Jan 19, 2014 at 5​:42 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

A different approach is to not use the shell, please let me know if the attached fixes the problem for you.

Your patch doesn't fix it for me - my perl doesn't exit on a SIGHUP.

Perhaps your patch + SIGTERM, or some other way to signal to the child
process that it's time to end? (Or send both signals?)

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 20, 2014

From @wolfsage

Whatever the solution, t/io/openpid.t suffers the same problem and can
be reduced to less than a second from its current 30 seconds.

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @Leont

On Mon Jan 20 06​:26​:02 2014, alh wrote​:

On Sun, Jan 19, 2014 at 5​:42 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

A different approach is to not use the shell, please let me know if
the attached fixes the problem for you.

Your patch doesn't fix it for me - my perl doesn't exit on a SIGHUP.

Perhaps your patch + SIGTERM, or some other way to signal to the child
process that it's time to end? (Or send both signals?)

-- Matthew Horsfall (alh)

What is the output of this command on your system?

perl -MPOSIX=sigprocmask,SIG_SETMASK -E 'sigprocmask(SIG_SETMASK, undef, my $set = POSIX​::SigSet->new); say join "", map $set->ismember($_), 1 .. 32'

Leon

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @wolfsage

On Mon, Jan 20, 2014 at 8​:10 PM, Leon Timmermans via RT
<perlbug-followup@​perl.org> wrote​:

On Mon Jan 20 06​:26​:02 2014, alh wrote​:

On Sun, Jan 19, 2014 at 5​:42 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

A different approach is to not use the shell, please let me know if
the attached fixes the problem for you.

Your patch doesn't fix it for me - my perl doesn't exit on a SIGHUP.

Perhaps your patch + SIGTERM, or some other way to signal to the child
process that it's time to end? (Or send both signals?)

-- Matthew Horsfall (alh)

What is the output of this command on your system?

perl -MPOSIX=sigprocmask,SIG_SETMASK -E 'sigprocmask(SIG_SETMASK, undef, my $set = POSIX​::SigSet->new); say join "", map $set->ismember($_), 1 .. 32'

mhorsfall@​tworivers​:~$ perl -MPOSIX=sigprocmask,SIG_SETMASK -E
'sigprocmask(SIG_SETMASK, undef, my $set = POSIX​::SigSet->new); say
join "", map $set->ismember($_), 1 .. 32'
00000000000000000000000000000000

mhorsfall@​tworivers​:~$ uname -a
Linux tworivers 3.8.0-31-generic #46~precise1-Ubuntu SMP Wed Sep 11
18​:21​:16 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @wolfsage

On Tue, Jan 21, 2014 at 9​:16 AM, Matthew Horsfall (alh)
<wolfsage@​gmail.com> wrote​:

mhorsfall@​tworivers​:~$ perl -MPOSIX=sigprocmask,SIG_SETMASK -E
'sigprocmask(SIG_SETMASK, undef, my $set = POSIX​::SigSet->new); say
join "", map $set->ismember($_), 1 .. 32'
00000000000000000000000000000000

mhorsfall@​tworivers​:~$ uname -a
Linux tworivers 3.8.0-31-generic #46~precise1-Ubuntu SMP Wed Sep 11
18​:21​:16 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

Oddly, my other ubuntu box (Which is ubuntu server instead of ubuntu
desktop) behaves correctly​:

mhorsfall@​dory​:~$ uname -a
Linux dory 3.8.0-29-generic #42~precise1-Ubuntu SMP Wed Aug 14
16​:19​:23 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @Leont

On Tue, Jan 21, 2014 at 3​:16 PM, Matthew Horsfall (alh)
<wolfsage@​gmail.com>wrote​:

What is the output of this command on your system?

perl -MPOSIX=sigprocmask,SIG_SETMASK -E 'sigprocmask(SIG_SETMASK, undef,
my $set = POSIX​::SigSet->new); say join "", map $set->ismember($_), 1 .. 32'

mhorsfall@​tworivers​:~$ perl -MPOSIX=sigprocmask,SIG_SETMASK -E
'sigprocmask(SIG_SETMASK, undef, my $set = POSIX​::SigSet->new); say
join "", map $set->ismember($_), 1 .. 32'
00000000000000000000000000000000

And how about « perl -E 'say $SIG{HUP}' » ?

Leon

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @wolfsage

On Tue, Jan 21, 2014 at 9​:37 AM, Leon Timmermans <fawaka@​gmail.com> wrote​:

And how about « perl -E 'say $SIG{HUP}' » ?

mhorsmhorsfall@​tworivers​:~/perl-1$ ./perl -Ilib -E 'say $SIG{HUP}'
IGNORE

What's going on here?

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 21, 2014

From @Leont

On Tue, Jan 21, 2014 at 4​:08 PM, Matthew Horsfall (alh)
<wolfsage@​gmail.com>wrote​:

On Tue, Jan 21, 2014 at 9​:37 AM, Leon Timmermans <fawaka@​gmail.com> wrote​:

And how about « perl -E 'say $SIG{HUP}' » ?

mhorsmhorsfall@​tworivers​:~/perl-1$ ./perl -Ilib -E 'say $SIG{HUP}'
IGNORE

What's going on here?

Something sets up your environment to ignore SIGHUP by default. Possibly a
«trap "" HUP» in you shell profile.

Leon

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 22, 2014

From @tonycoz

On Mon Jan 20 06​:26​:02 2014, alh wrote​:

On Sun, Jan 19, 2014 at 5​:42 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

A different approach is to not use the shell, please let me know if
the attached fixes the problem for you.

Your patch doesn't fix it for me - my perl doesn't exit on a SIGHUP.

Perhaps your patch + SIGTERM, or some other way to signal to the child
process that it's time to end? (Or send both signals?)

The attached patch allows closepid() to finish quickly for me in a shell with C< trap "" HUP >.

This makes sure that $SIG{HUP} is DEFAULT before we expect it to work, though perhaps it should just skip when $SIG{HUP} and complain your system is broken.

Preventing openpid.t from delaying processing would require similar changes I think - using 3+ arg open for the pipe opens (to avoid the extra processes) and preventing $SIG{HUP} being ignored.

Note that the problem in both cases isn't that (open|close)pid.t is waiting, but that harness is waiting for the stdout handle from the children to be closed.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 22, 2014

From @tonycoz

0001-perl-121028-avoid-creating-a-shell-process.patch
From 0bc4d3b37fc2ae0ffd41dc5d358e50f33da7ff33 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 22 Jan 2014 15:14:59 +1100
Subject: [PATCH] [perl #121028] avoid creating a shell process

---
 t/io/closepid.t |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/io/closepid.t b/t/io/closepid.t
index aa937f5..a90db68 100644
--- a/t/io/closepid.t
+++ b/t/io/closepid.t
@@ -16,10 +16,11 @@ watchdog(10, $^O eq 'MSWin32' ? "alarm" : '');
 use Config;
 $| = 1;
 $SIG{PIPE} = 'IGNORE';
+# work around a shell set to ignore HUP
+$SIG{HUP} = 'DEFAULT';
 $SIG{HUP} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
-$perl .= qq[ "-I../lib"];
 
 my $killsig = 'HUP';
 $killsig = 1 unless $Config{sig_name} =~ /\bHUP\b/;
@@ -34,7 +35,7 @@ SKIP:
     # close on the original of a popen handle dupped to a standard handle
     # would wait4pid(0, ...)
     open my $savein, "<&", \*STDIN;
-    my $pid = open my $fh1, qq/$perl -e "sleep 50" |/;
+    my $pid = open my $fh1, "-|", $perl, "-e", "sleep 50";
     ok($pid, "open a pipe");
     # at this point PL_fdpids[fileno($fh1)] is the pid of the new process
     ok(open(STDIN, "<&=", $fh1), "dup the pipe");
-- 
1.7.10.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 24, 2014

From @wolfsage

On Tue, Jan 21, 2014 at 11​:46 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

This makes sure that $SIG{HUP} is DEFAULT before we expect it to work, though perhaps it should just skip when $SIG{HUP} and complain your system is broken.

Preventing openpid.t from delaying processing would require similar changes I think - using 3+ arg open for the pipe opens (to avoid the extra processes) and preventing $SIG{HUP} being ignored.

Note that the problem in both cases isn't that (open|close)pid.t is waiting, but that harness is waiting for the stdout handle from the children to be closed.

Awesome, this works great for me.

How do you feel about applying this to other places where
HUP/open()/kill are combined for this sort of functionality?

If you're busy I'd be willing to throw this together.

Thanks,

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 28, 2014

From @tonycoz

On Fri Jan 24 05​:08​:05 2014, alh wrote​:

On Tue, Jan 21, 2014 at 11​:46 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

This makes sure that $SIG{HUP} is DEFAULT before we expect it to
work, though perhaps it should just skip when $SIG{HUP} and complain
your system is broken.

Preventing openpid.t from delaying processing would require similar
changes I think - using 3+ arg open for the pipe opens (to avoid the
extra processes) and preventing $SIG{HUP} being ignored.

Note that the problem in both cases isn't that (open|close)pid.t is
waiting, but that harness is waiting for the stdout handle from the
children to be closed.

Awesome, this works great for me.

How do you feel about applying this to other places where
HUP/open()/kill are combined for this sort of functionality?

If you're busy I'd be willing to throw this together.

The attached patch allows openpid.t to finish quickly under harness on Linux - *but* fails horribly on Win32​:

C​:\Users\tony\dev\perl\git\perl\t>.\perl io\openpid.t
1..10
List form of pipe open not implemented at io\openpid.t line 56.
# Looks like you planned 10 tests but ran 0.

Implementing list form pipes on Win32 is complicated by there being no standard command-line argument parsing mechanism - apparently VB for example uses different rules to the CRT.

This isn't a problem for closepid.t, since that test is skipped on Win32.

An option might be to implement list form pipes for Win32, but I'm not sure I want to jump down that rabbit hole.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 28, 2014

From @tonycoz

0001-perl-121028-avoid-creating-a-shell-process.patch
From 0fd13b4b4422d848e9b1dcaacf8800a17a1f7cdb Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 28 Jan 2014 15:52:22 +1100
Subject: [PATCH] [perl #121028] avoid creating a shell process

---
 t/io/closepid.t |    5 +++--
 t/io/openpid.t  |   20 +++++++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/t/io/closepid.t b/t/io/closepid.t
index aa937f5..a90db68 100644
--- a/t/io/closepid.t
+++ b/t/io/closepid.t
@@ -16,10 +16,11 @@ watchdog(10, $^O eq 'MSWin32' ? "alarm" : '');
 use Config;
 $| = 1;
 $SIG{PIPE} = 'IGNORE';
+# work around a shell set to ignore HUP
+$SIG{HUP} = 'DEFAULT';
 $SIG{HUP} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
-$perl .= qq[ "-I../lib"];
 
 my $killsig = 'HUP';
 $killsig = 1 unless $Config{sig_name} =~ /\bHUP\b/;
@@ -34,7 +35,7 @@ SKIP:
     # close on the original of a popen handle dupped to a standard handle
     # would wait4pid(0, ...)
     open my $savein, "<&", \*STDIN;
-    my $pid = open my $fh1, qq/$perl -e "sleep 50" |/;
+    my $pid = open my $fh1, "-|", $perl, "-e", "sleep 50";
     ok($pid, "open a pipe");
     # at this point PL_fdpids[fileno($fh1)] is the pid of the new process
     ok(open(STDIN, "<&=", $fh1), "dup the pipe");
diff --git a/t/io/openpid.t b/t/io/openpid.t
index 946fa5e..de75648 100644
--- a/t/io/openpid.t
+++ b/t/io/openpid.t
@@ -23,11 +23,15 @@ watchdog(15, $^O eq 'MSWin32' ? "alarm" : '');
 use Config;
 $| = 1;
 $SIG{PIPE} = 'IGNORE';
+# reset the handler in case the shell has set a broken default
+$SIG{HUP} = 'DEFAULT';
 $SIG{HUP} = 'IGNORE' if $^O eq 'interix';
 
 my $perl = which_perl();
 $perl .= qq[ "-I../lib"];
 
+my @perl = ( which_perl(), "-I../lib" );
+
 #
 # commands run 4 perl programs.  Two of these programs write a
 # short message to STDOUT and exit.  Two of these programs
@@ -35,16 +39,22 @@ $perl .= qq[ "-I../lib"];
 # the other reader reads one line, waits a few seconds and then
 # exits to test the waitpid function.
 #
-$cmd1 = qq/$perl -e "\$|=1; print qq[first process\\n]; sleep 30;"/;
-$cmd2 = qq/$perl -e "\$|=1; print qq[second process\\n]; sleep 30;"/;
+# Using 4+ arg open for the children that sleep so that that we're
+# killing the perl process instead of an intermediate shell, this
+# allows harness to see the file handles closed sooner.  I didn't
+# convert them all since I wanted 3-arg open to continue to be
+# exercised here.
+#
+@cmd1 = ( @perl, "-e", "\$|=1; print qq[first process\\n]; sleep 30;" );
+@cmd2 = ( @perl, "-e", "\$|=1; print qq[second process\\n]; sleep 30;" );
 $cmd3 = qq/$perl -e "print <>;"/; # hangs waiting for end of STDIN
 $cmd4 = qq/$perl -e "print scalar <>;"/;
 
-#warn "#$cmd1\n#$cmd2\n#$cmd3\n#$cmd4\n";
+#warn "#@cmd1\n#@cmd2\n#$cmd3\n#$cmd4\n";
 
 # start the processes
-ok( $pid1 = open(FH1, "$cmd1 |"), 'first process started');
-ok( $pid2 = open(FH2, "$cmd2 |"), '    second' );
+ok( $pid1 = open(FH1, "-|", @cmd1), 'first process started');
+ok( $pid2 = open(FH2, "-|", @cmd2), '    second' );
 {
     no warnings 'once';
     ok( $pid3 = open(FH3, "| $cmd3"), '    third'  );
-- 
1.7.10.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 30, 2014

From @Leont

On Tue, Jan 28, 2014 at 6​:48 AM, Tony Cook via RT <perlbug-followup@​perl.org

wrote​:

C​:\Users\tony\dev\perl\git\perl\t>.\perl io\openpid.t
1..10
List form of pipe open not implemented at io\openpid.t line 56.
# Looks like you planned 10 tests but ran 0.

Implementing list form pipes on Win32 is complicated by there being no
standard command-line argument parsing mechanism - apparently VB for
example uses different rules to the CRT.

This isn't a problem for closepid.t, since that test is skipped on Win32.

An option might be to implement list form pipes for Win32, but I'm not
sure I want to jump down that rabbit hole.

I think that would be highly desirable anyway. I'd second only to unicode
paths in "parts of perl where our system interfaces suck on Windows".

Leon

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 3, 2014

From @tonycoz

On Tue Jan 21 20​:46​:07 2014, tonyc wrote​:

The attached patch allows closepid() to finish quickly for me in a
shell with C< trap "" HUP >.

This makes sure that $SIG{HUP} is DEFAULT before we expect it to work,
though perhaps it should just skip when $SIG{HUP} and complain your
system is broken.

I've applied the closepid.t change to blead as af1f7e2.

I don't think it needs a perldelta entry.

I'll leave this ticket open until list form pipe is available on Win32 and a similar change can be made to io/openpid.t

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 15, 2018

From @tonycoz

On Mon, 03 Feb 2014 15​:41​:46 -0800, tonyc wrote​:

On Tue Jan 21 20​:46​:07 2014, tonyc wrote​:

The attached patch allows closepid() to finish quickly for me in a
shell with C< trap "" HUP >.

This makes sure that $SIG{HUP} is DEFAULT before we expect it to
work,
though perhaps it should just skip when $SIG{HUP} and complain your
system is broken.

I've applied the closepid.t change to blead as
af1f7e2.

I don't think it needs a perldelta entry.

I'll leave this ticket open until list form pipe is available on Win32
and a similar change can be made to io/openpid.t

Which was done in b6811f8.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 15, 2018

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.