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

process group kill on Win32 broken in 5.17.2, regression 5.18 #13595

Open
p5pRT opened this issue Feb 11, 2014 · 52 comments
Open

process group kill on Win32 broken in 5.17.2, regression 5.18 #13595

p5pRT opened this issue Feb 11, 2014 · 52 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Feb 11, 2014

Migrated from rt.perl.org#121230 (status was 'open')

Searchable as RT121230$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 11, 2014

From @bulk88

Created by @bulk88

Patch
http​://perl5.git.perl.org/perl.git/c2fd40cb025bad682ee44b1ccb8ea501ed51924d
from 5.17.2 and https://rt-archive.perl.org/perl5/Ticket/Display.html?id=112990 broke
the process group (actually process tree, since process groups don't
exist on Win32, I'll call it a process group for the remainder of the
ticket, -signal kills a process tree on Win32 Perl, not a process group)
kill feature of Win32 Perl's kill() PP func. I attached a sample script
of the bug, YMMV on what it does on POSIX. IDK and IDC what the script
does on POSIX. If the script can't kill the child proc, you will need to
manually kill the proc with Task Manager. The script returns success on
5.12, 5.14, 5.16, but fails on 5.18 and on blead. The fix for this will
need to go into 5.18 maint if 5.18 maint is still open by the time there
is a patch.

----------------------------------
DllExport int
win32_kill(int pid, int sig)
{
----------------------------------

On 5.16 and older, when running the test script, sig is -, and pid +.
That patch changed it so sig is + and pid is -. A -pid on Win32 Perl is
a psuedofork fake-PID. So on post-patch, entry win32_kill checks if PID
is neg, takes the fake-PID branch (
http​://perl5.git.perl.org/perl.git/blob/d048ecb4d187ebbafce763f1e0437120c6726967​:/win32/win32.c#1372
) looks up the bad -PID in the fake-PID table, doesn't find it, and
returns with does
----------------------------------
  errno = EINVAL;
  return -1;
}
----------------------------------

Callstacks from the 2nd system proc in test script.
----------------------------------

perl516.dll!win32_kill(int pid=2288, int sig=-9) Line 1277 C
  perl516.dll!PerlProcKillpg(IPerlProc * piPerl=0x00424700, int
pid=2288, int sig=9) Line 1607 + 0xf bytes C++
  perl516.dll!Perl_apply(interpreter * my_perl=0x01b94004, long
type=303, sv * * mark=0x01b99934, sv * * sp=0x01b99934) Line 1746 +
0x20 bytes C
  perl516.dll!Perl_pp_chown(interpreter * my_perl=0x01b94004) Line
3558 + 0x24 bytes C
  perl516.dll!Perl_runops_debug(interpreter * my_perl=0x01b94004)
Line 2119 + 0xf bytes C
  perl516.dll!S_run_body(interpreter * my_perl=0x01b94004, long
oldscope=1) Line 2402 + 0xf bytes C
  perl516.dll!perl_run(interpreter * my_perl=0x01b94004) Line 2320 +
0xd bytes C
  perl516.dll!RunPerl(int argc=3, char * * argv=0x004231f0, char * *
env=0x00426448) Line 270 + 0x9 bytes C++
  perl.exe!main(int argc=3, char * * argv=0x004231f0, char * *
env=0x00423600) Line 23 + 0x12 bytes C
  perl.exe!__tmainCRTStartup() Line 582 + 0x17 bytes C
  kernel32.dll!_BaseProcessStart@​4() + 0x28 bytes
---------------------------------
perl519.dll!win32_kill(int pid=-3464, int sig=9) Line 1369 C
  perl519.dll!PerlProcKill(IPerlProc * piPerl=0x01b67950, int
pid=-3464, int sig=9) Line 1599 + 0xd bytes C++
  perl519.dll!Perl_apply(interpreter * my_perl=0x01b6858c, long
type=304, sv * * mark=0x01b6cfdc, sv * * sp=0x01b6cfdc) Line 1797 +
0x20 bytes C
  perl519.dll!Perl_pp_chown(interpreter * my_perl=0x01b6858c) Line
3509 + 0x24 bytes C
  perl519.dll!Perl_runops_standard(interpreter * my_perl=0x01b6858c)
Line 42 + 0xc bytes C
  perl519.dll!S_run_body(interpreter * my_perl=0x01b6858c, long
oldscope=1) Line 2446 + 0xf bytes C
  perl519.dll!perl_run(interpreter * my_perl=0x01b6858c) Line 2365 C
  perl519.dll!RunPerl(int argc=3, char * * argv=0x01b631d0, char * *
env=0x01b63248) Line 270 + 0x9 bytes C++
  perl.exe!__tmainCRTStartup() Line 582 + 0x17 bytes C
  kernel32.dll!_BaseProcessStart@​4() + 0x28 bytes
---------------------------------

Another problem is that this removal in the patch is probably
unacceptable on Win32 Perl.

------------------------------------------------
- APPLY_TAINT_PROPER();
-#ifdef HAS_KILLPG
- if (PerlProc_killpg(proc,val)) /* BSD */
-#else
- if (PerlProc_kill(-proc,val)) /* SYSV */
-#endif
- tot--;
------------------------------------------------

Win32 Perl defines PerlProcKillpg as this

------------------------------------------------
int
PerlProcKillpg(struct IPerlProc* piPerl, int pid, int sig)
{
  return win32_kill(pid, -sig);
}
-------------------------------------------------

About the test script included, both system()ed perls are launched and
hang off cmd.exe (win32 shell) processes. Why they are launched in
shells is a separate issue for another ticket.

How this regression was found? This report came from trying to eliminate
harness from hanging/waiting against a child .t proc's watchdog()'s FULL
time. Since system() returned the shell's PID, not the watchdog perl
proc's PID, killing the shell didn't kill the watchdog process, and
harness hung/waited 300 seconds in some cases (or whatever the watchdog
is set for, and it is 300 seconds in some places like re/susbt.t) for
the watchdog to timeout and naturally exit. There are a number of bugs
and solutions in the watchdog problem, one attempt I tried was to kill
the process tree (the cmd.exe and the watchdog perl proc underneath it)
instead of just the shell. That didn't work in blead but it did from a
old system perl, which lead to this ticket.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.19.8:

Configured by Owner at Thu Dec 26 04:12:47 2013.

Summary of my perl5 (revision 5 version 19 subversion 8) configuration:
  Derived from:
  Platform:
    osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -G7 -GL 
-DWIN32 -D_CONSOLE -DNO_STRICT  -DPERL_TEXTMODE_SCRIPTS 
-DPERL_HASH_FUNC_ONE_AT_A_TIME -DPERL_IMPLICIT_CONTEXT 
-DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T',
    optimize='-O1 -MD -Zi -DNDEBUG -G7 -GL',
    cppflags='-DWIN32'
    ccversion='13.10.6030', gccversion='', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', 
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf 
-ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'
    libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
    libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib  
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib 
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib 
winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib 
oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl519.lib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug 
-opt:ref,icf -ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'

Locally applied patches:
    uncommitted-changes


@INC for perl 5.19.8:
    C:/perl519/site/lib
    C:/perl519/lib
    .


Environment for perl 5.19.8:
    HOME (unset)
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=C:\perl519\bin;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET 
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
    PERL_BADLANG (unset)
    SHELL (unset)


@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 11, 2014

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 11, 2014

From @bulk88

C​:\>perl -V
Summary of my perl5 (revision 5 version 19 subversion 4) configuration​:
  Derived from​:
  Platform​:
  osname=MSWin32, osvers=5.2, archname=MSWin32-x86-multi-thread
  uname=''
  config_args='undef'
  hint=recommended, useposix=true, d_sigaction=undef
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cl', ccflags ='-nologo -GF -W3 -Od -MD -Zi -DNDEBUG -GS- -DWIN32 -D_CONS
OLE -DNO_STRICT -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -DPERL_T
EXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO',
  optimize='-Od -MD -Zi -DNDEBUG -GS-',
  cppflags='-DWIN32'
  ccversion='15.00.30729.01', gccversion='', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', lseeksi
ze=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='link', ldflags ='-nologo -nodefaultlib -debug -opt​:ref,icf -libpath​:"c​:
\p519\lib\CORE" -machine​:x86 "/manifestdependency​:type='Win32' name='Microsoft.
Windows.Common-Controls' version='6.0.0.0' processorArchitecture='*' publicKeyTo
ken='6595b64144ccf1df' language='*'"'
  libpth=\lib
  libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.l
ib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32
.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.
lib
  perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg
32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws
2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msv
crt.lib
  libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl519.lib
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug -opt​:ref,icf -
libpath​:"c​:\p519\lib\CORE" -machine​:x86 "/manifestdependency​:type='Win32' name=
'Microsoft.Windows.Common-Controls' version='6.0.0.0' processorArchitecture='*'
publicKeyToken='6595b64144ccf1df' language='*'"'

Characteristics of this binary (from libperl)​:
  Compile-time options​: HAS_TIMES HAVE_INTERP_INTERN MULTIPLICITY
  PERLIO_LAYERS PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  PERL_IMPLICIT_CONTEXT PERL_IMPLICIT_SYS
  PERL_MALLOC_WRAP PERL_NEW_COPY_ON_WRITE
  PERL_PRESERVE_IVUV USE_ITHREADS USE_LARGE_FILES
  USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC USE_PERLIO USE_PERL_ATOF
  Locally applied patches​:
  uncommitted-changes
  Built under MSWin32
  Compiled at Oct 3 2013 15​:18​:14
  @​INC​:
  C​:/p519/site/lib
  C​:/p519/lib
  .

C​:\>perl "C​:\Documents and Settings\Administrator\Desktop\kill.pl"

C​:\>couldnt kill

C​:\>p3216

C​:\>set PATH=*REMOVED*

C​:\>set PERL_JSON_BACKEND=JSON​::XS

C​:\>set PERL_YAML_BACKEND=YAML
C​:\>perl -V
Summary of my perl5 (revision 5 version 16 subversion 3) configuration​:

  Platform​:
  osname=MSWin32, osvers=5.2, archname=MSWin32-x86-multi-thread
  uname=''
  config_args='undef'
  hint=recommended, useposix=true, d_sigaction=undef
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cl', ccflags ='-nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE -
DNO_STRICT -DPERL_TEXTMODE_SCRIPTS -DUSE_SITECUSTOMIZE -DPERL_IMPLICIT_CONTEXT -
DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T',
  optimize='-MD -Zi -DNDEBUG -O1',
  cppflags='-DWIN32'
  ccversion='15.0.30729', gccversion='', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', lseeksi
ze=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='link', ldflags ='-nologo -nodefaultlib -debug -opt​:ref,icf -libpath​:"C​:
\Perl3216\lib\CORE" -machine​:x86'
  libpth=\lib
  libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.l
ib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32
.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.
lib
  perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg
32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws
2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msv
crt.lib
  libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl516.lib
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug -opt​:ref,icf -
libpath​:"C​:\Perl3216\lib\CORE" -machine​:x86'

Characteristics of this binary (from libperl)​:
  Compile-time options​: HAS_TIMES HAVE_INTERP_INTERN MULTIPLICITY
  PERLIO_LAYERS PERL_DONT_CREATE_GVSV
  PERL_IMPLICIT_CONTEXT PERL_IMPLICIT_SYS
  PERL_MALLOC_WRAP PERL_PRESERVE_IVUV PL_OP_SLAB_ALLOC
  USE_ITHREADS USE_LARGE_FILES USE_LOCALE
  USE_LOCALE_COLLATE USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC USE_PERLIO USE_PERL_ATOF
  USE_SITECUSTOMIZE
  Locally applied patches​:
  ActivePerl Build 1603 [296746]
  Built under MSWin32
  Compiled at Mar 13 2013 11​:29​:21
  %ENV​:
  PERL_JSON_BACKEND="JSON​::XS"
  PERL_YAML_BACKEND="YAML"
  @​INC​:
  C​:/Perl3216/site/lib
  C​:/Perl3216/lib
  .

C​:\>perl "C​:\Documents and Settings\Administrator\Desktop\kill.pl"

C​:\>kill sucessful

C​:\>

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 12, 2014

From @steve-m-hay

On 11 February 2014 18​:52, bulk88 <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by bulk88
# Please include the string​: [perl #121230]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121230 >

This is a bug report for perl from bulk88@​hotmail.com,
generated with the help of perlbug 1.39 running under perl 5.19.8.

-----------------------------------------------------------------
[Please describe your issue here]

Patch
http​://perl5.git.perl.org/perl.git/c2fd40cb025bad682ee44b1ccb8ea501ed51924d
from 5.17.2 and https://rt-archive.perl.org/perl5/Ticket/Display.html?id=112990 broke
the process group (actually process tree, since process groups don't
exist on Win32, I'll call it a process group for the remainder of the
ticket, -signal kills a process tree on Win32 Perl, not a process group)
kill feature of Win32 Perl's kill() PP func.
[...]

How this regression was found? This report came from trying to eliminate
harness from hanging/waiting against a child .t proc's watchdog()'s FULL
time. Since system() returned the shell's PID, not the watchdog perl
proc's PID, killing the shell didn't kill the watchdog process, and
harness hung/waited 300 seconds in some cases (or whatever the watchdog
is set for, and it is 300 seconds in some places like re/susbt.t) for
the watchdog to timeout and naturally exit. There are a number of bugs
and solutions in the watchdog problem, one attempt I tried was to kill
the process tree (the cmd.exe and the watchdog perl proc underneath it)
instead of just the shell. That didn't work in blead but it did from a
old system perl, which lead to this ticket.

Thank you for finding this! It's bugged me for ages that
re/subst_amp.t and re/subst_wamp.t both hang for ages when running the
test suite, but I've never got round to looking into why :-/

Clearly I should have shouted because this is a nasty regression.

This one really needs fixing. Should it even be a blocker for 5.20?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 12, 2014

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 12, 2014

From @bulk88

On Wed Feb 12 01​:07​:10 2014, shay wrote​:

from 5.17.2 and https://rt-archive.perl.org/perl5/Ticket/Display.html?id=112990

Thank you for finding this! It's bugged me for ages that
re/subst_amp.t and re/subst_wamp.t both hang for ages when running the
test suite, but I've never got round to looking into why :-/

I will be very clear, this ticket isn't about that watchdog hang. Fixing kill() process group will not unhang that test.

Clearly I should have shouted because this is a nasty regression.

This one really needs fixing. Should it even be a blocker for 5.20?

Yes because the feature worked in 5.12 to 5.16. IDK if it existed in < 5.12. I can't fix this because it is a POSIX, and I IDK enough in that area. I do not understand the complaint in #112990 and solution in there. Therefore my opinion is to git revert that commit, but that probably is not the right solution. If I do try to code it, I will probably break non-Win32 builds. Therefore I didn't offer a patch and I only supplied a test script and the details in the OP.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 22, 2014

From @bulk88

Bump. If there aren't any ideas from the posix people to fix this, the only option I see is to create a patch to revert c2fd40c .

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 23, 2014

From @iabyn

On Sat, Feb 22, 2014 at 01​:50​:12AM -0800, bulk88 via RT wrote​:

Bump. If there aren't any ideas from the posix people to fix this, the only option I see is to create a patch to revert c2fd40c .

I've pushed a fix as smoke-me/davem/killpg. I haven't of course tested it
on Win32. If you're able to confirm it works, and if the smokes are ok,
I'll merge it into blead. It should then be a candidate for maint-5.18.

Are you in a position to contribute Win32-specific tests for this?

--
Standards (n). Battle insignia or tribal totems.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 25, 2014

From @bulk88

On Sun Feb 23 05​:57​:55 2014, davem wrote​:

I've pushed a fix as smoke-me/davem/killpg. I haven't of course tested
it
on Win32. If you're able to confirm it works, and if the smokes are
ok,
I'll merge it into blead. It should then be a candidate for maint-
5.18.

Are you in a position to contribute Win32-specific tests for this?

Yes. It will be a day or 2 for me to try it. I'm not sure whether to add the test to t/op/kill0.t or t/win32/system.t.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @bulk88

On Mon Feb 24 19​:23​:45 2014, bulk88 wrote​:

While writing a test for this, I discovered another bug or issue in Win32 PG kill. PG kill is supposed to return the number of processes "signaled" (is this a synonym for "killed" on Win32 platform?) per perldoc


Sends a signal to a list of processes. Returns the number of processes successfully signaled (which is not necessarily the same as the number actually killed).


My testing shows the retval in PP of PG kill in 5.12.2 is always 1.

from win32_kill()


  if (child >= 0) {
  if (my_kill(pid, sig)) {<<<<<<<<<<<<kill count lost
  DWORD exitcode = 0;
  if (GetExitCodeProcess(w32_child_handles[child], &exitcode) &&
  exitcode != STILL_ACTIVE)
  {
  remove_dead_process(child);
  }
  return 0;
  }
  }


Perl_apply


  if (val < 0) {
  val = -val;
  while (++mark <= sp) {
  I32 proc;
  if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
  Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
  proc = SvIV(*mark);
  APPLY_TAINT_PROPER();
#ifdef HAS_KILLPG
  if (PerlProc_killpg(proc,val)) /* BSD */ <<<<<<<<<<<<<no way to return kill count, but posix killpg doesn't return process count, just error code
#else
  if (PerlProc_kill(-proc,val)) /* SYSV */
#endif


Is this something to be fixed or it is correct behavior that you can't tell how many processes were in the group from a kill on a PG?

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @iabyn

On Tue, Feb 25, 2014 at 05​:10​:33PM -0800, bulk88 via RT wrote​:

While writing a test for this, I discovered another bug or issue in
Win32 PG kill. PG kill is supposed to return the number of processes
"signaled" (is this a synonym for "killed" on Win32 platform?) per
perldoc
------------------------------------------------
Sends a signal to a list of processes. Returns the number of processes
successfully signaled (which is not necessarily the same as the number
actually killed).
-----------------------------------------------

My testing shows the retval in PP of PG kill in 5.12.2 is always 1.

I think the perl docs are misleading. It's not supposed to indicate
the total *processes* killed - UNIX certainly doesn't provide this info
when killing groups - but rather the number of 'kills' in the list
that were successful. i.e.

  kill $SIG, $proc1, $proc2, $no_such_process;

returns 2.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @bulk88

On Sun Feb 23 05​:57​:55 2014, davem wrote​:

On Sat, Feb 22, 2014 at 01​:50​:12AM -0800, bulk88 via RT wrote​:

Bump. If there aren't any ideas from the posix people to fix this,
the only option I see is to create a patch to revert
c2fd40c .

I've pushed a fix as smoke-me/davem/killpg. I haven't of course tested
it
on Win32. If you're able to confirm it works, and if the smokes are
ok,
I'll merge it into blead. It should then be a candidate for maint-
5.18.

Are you in a position to contribute Win32-specific tests for this?

Test patch attached. Not fully smoked on Win32, but "perl harness op/kill0.t" passes. I'm not sure if the unix file perms on the new file in the patch are correct since my git is Win32.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @bulk88

0001-RT-121230-create-tests-for-process-group-kill-on-Win.patch
From 7062d2991e08b2b516db88450d4337d83910d462 Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Wed, 26 Feb 2014 17:31:18 -0500
Subject: [PATCH] RT #121230, create tests for process group kill on Win32

---
 t/op/kill0.t     |   41 ++++++++++++++++++++++++++++++++++++++++-
 t/op/kill0_child |    9 +++++++++
 2 files changed, 49 insertions(+), 1 deletions(-)
 create mode 100644 t/op/kill0_child

diff --git a/t/op/kill0.t b/t/op/kill0.t
index d3ef8f7..4012761 100644
--- a/t/op/kill0.t
+++ b/t/op/kill0.t
@@ -13,8 +13,9 @@ BEGIN {
 }
 
 use strict;
+use Config;
 
-plan tests => 6;
+plan tests => 9;
 
 ok( kill(0, $$), 'kill(0, $pid) returns true if $pid exists' );
 
@@ -50,3 +51,41 @@ for my $case ( @bad_pids ) {
   $x =~ /(\d+)/;
   ok(eval { kill 0, $1 }, "can kill a number string in a magic variable");
 }
+
+SKIP: {
+  skip 'custom process group kill() only on Win32', 3 if ($^O ne 'MSWin32');
+  #create 2 child processes, an outer one created by kill0.t, and an inner one
+  #created by outer this allows the test to fail if only the outer one was
+  #killed, since the inner will stay around and eventually print failed and
+  #out of sequence TAP to harness
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  eval q|END{unlink('killchildstarted');}|;
+  my $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  #ways to break this test manually, change '-KILL' to 'KILL', change $pid to a
+  #bogus number
+  is(kill('-KILL', $pid), 1, 'process group kill, named signal');
+
+  my ($i, %signo, @signame, $sig_name) = 0;
+  ($sig_name = $Config{sig_name}) || die "No signals?";
+  foreach my $name (split(' ', $sig_name)) {
+    $signo{$name} = $i;
+    $signame[$i] = $name;
+    $i++;
+  }
+  ok(scalar keys %signo > 1 && exists $signo{KILL}, '$Config{sig_name} parsed correctly');
+  die q|A child proc wasn't killed and did cleanup on its own| if ! -e 'killchildstarted';
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  #no END block, done earlier
+  $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  is(kill(-$signo{KILL}, $pid), 1, 'process group kill, numeric signal');
+}
diff --git a/t/op/kill0_child b/t/op/kill0_child
new file mode 100644
index 0000000..c3d5eb2
--- /dev/null
+++ b/t/op/kill0_child
@@ -0,0 +1,9 @@
+#$ARGV[0] is filename used to notify parent .t perl proc that all PIDs are
+#started in the process tree
+#number 9999/9998 is eye catching
+system(1, $^X, '-e', 'sleep 5; print qq|not ok 9999 - inner child process wasn\'t killed\n|;');
+system('echo outer child started > "'.$ARGV[0].'"');
+sleep 5;
+#execution won't be reached if test successful
+print "not ok 9998 - outer child process wasn\'t killed\n";
+unlink($ARGV[0]);
-- 
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @bulk88

On Wed Feb 26 06​:04​:15 2014, davem wrote​:

On Tue, Feb 25, 2014 at 05​:10​:33PM -0800, bulk88 via RT wrote​:

While writing a test for this, I discovered another bug or issue in
Win32 PG kill. PG kill is supposed to return the number of processes
"signaled" (is this a synonym for "killed" on Win32 platform?) per
perldoc
------------------------------------------------
Sends a signal to a list of processes. Returns the number of processes
successfully signaled (which is not necessarily the same as the number
actually killed).
-----------------------------------------------

My testing shows the retval in PP of PG kill in 5.12.2 is always 1.

I think the perl docs are misleading. It's not supposed to indicate
the total *processes* killed - UNIX certainly doesn't provide this info
when killing groups - but rather the number of 'kills' in the list
that were successful. i.e.

kill $SIG\, $proc1\, $proc2\, $no\_such\_process;

returns 2.

Patch for POD.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2014

From @bulk88

0001-clarify-kill-s-POD.patch
From 8a0503965f4d6e9165156115692d8f7a8338495c Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Wed, 26 Feb 2014 18:14:24 -0500
Subject: [PATCH] clarify kill's POD

part of RT #121230
---
 pod/perlfunc.pod |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/pod/perlfunc.pod b/pod/perlfunc.pod
index 14c5171..8a3aaab 100644
--- a/pod/perlfunc.pod
+++ b/pod/perlfunc.pod
@@ -3218,7 +3218,7 @@ X<kill> X<signal>
 
 Sends a signal to a list of processes.  Returns the number of
 processes successfully signaled (which is not necessarily the
-same as the number actually killed).
+same as the number actually killed) from the list of processes supplied.
 
     $cnt = kill 'HUP', $child1, $child2;
     kill 'KILL', @goners;
-- 
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 27, 2014

From @bulk88

On Wed Feb 26 14​:40​:54 2014, bulk88 wrote​:

Test patch attached. Not fully smoked on Win32, but "perl harness
op/kill0.t" passes. I'm not sure if the unix file perms on the new
file in the patch are correct since my git is Win32.

This patch is broken. Fails manifest.t and I forgot perldelta.pod. I have manifest.t fixed, but I am having a perldelta.pod and podcheck.t problem that is being addressed in a separate ML thread. So wait until I have a new patch.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 27, 2014

From @bulk88

On Wed Feb 26 23​:06​:03 2014, bulk88 wrote​:

This patch is broken. Fails manifest.t and I forgot perldelta.pod. I
have manifest.t fixed, but I am having a perldelta.pod and podcheck.t
problem that is being addressed in a separate ML thread. So wait until
I have a new patch.

New patch attached.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 27, 2014

From @bulk88

0001-RT-121230-create-tests-for-process-group-kill-on-Win.patch
From dc146462729d13b3038d9a58ac6291bc2e09f77f Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Thu, 27 Feb 2014 04:00:43 -0500
Subject: [PATCH] RT #121230, create tests for process group kill on Win32

---
 MANIFEST          |    3 ++-
 pod/perldelta.pod |    9 +++++++--
 t/op/kill0.t      |   41 ++++++++++++++++++++++++++++++++++++++++-
 t/op/kill0_child  |    9 +++++++++
 4 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100644 t/op/kill0_child

diff --git a/MANIFEST b/MANIFEST
index 9fcb518..e7ab59a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5238,7 +5238,8 @@ t/op/index.t			See if index works
 t/op/index_thr.t		See if index works in another thread
 t/op/int.t			See if int works
 t/op/join.t			See if join works
-t/op/kill0.t			See if kill(0, $pid) works
+t/op/kill0_child		Process tree script that is kill()ed
+t/op/kill0.t			See if kill works
 t/op/kvaslice.t			See if index/value array slices work
 t/op/kvhslice.t			See if key/value hash slices work
 t/op/lc.t			See if lc, uc, lcfirst, ucfirst, quotemeta work
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index ab26873..f298d47 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -319,11 +319,16 @@ and compilation changes or changes in portability/compatibility.  However,
 changes within modules for platforms should generally be listed in the
 L</Modules and Pragmata> section.
 
+=head3 Win32
+
 =over 4
 
-=item XXX-some-platform
+=item *
 
-XXX
+Killing a process tree with L<perlfunc/kill> and a negative signal, was broken
+starting in 5.17.2. In this bug, C<kill> always returned 0 for a negative
+signal even for valid PIDs, and no processes were terminated. This has been
+fixed [perl #121230].
 
 =back
 
diff --git a/t/op/kill0.t b/t/op/kill0.t
index d3ef8f7..4012761 100644
--- a/t/op/kill0.t
+++ b/t/op/kill0.t
@@ -13,8 +13,9 @@ BEGIN {
 }
 
 use strict;
+use Config;
 
-plan tests => 6;
+plan tests => 9;
 
 ok( kill(0, $$), 'kill(0, $pid) returns true if $pid exists' );
 
@@ -50,3 +51,41 @@ for my $case ( @bad_pids ) {
   $x =~ /(\d+)/;
   ok(eval { kill 0, $1 }, "can kill a number string in a magic variable");
 }
+
+SKIP: {
+  skip 'custom process group kill() only on Win32', 3 if ($^O ne 'MSWin32');
+  #create 2 child processes, an outer one created by kill0.t, and an inner one
+  #created by outer this allows the test to fail if only the outer one was
+  #killed, since the inner will stay around and eventually print failed and
+  #out of sequence TAP to harness
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  eval q|END{unlink('killchildstarted');}|;
+  my $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  #ways to break this test manually, change '-KILL' to 'KILL', change $pid to a
+  #bogus number
+  is(kill('-KILL', $pid), 1, 'process group kill, named signal');
+
+  my ($i, %signo, @signame, $sig_name) = 0;
+  ($sig_name = $Config{sig_name}) || die "No signals?";
+  foreach my $name (split(' ', $sig_name)) {
+    $signo{$name} = $i;
+    $signame[$i] = $name;
+    $i++;
+  }
+  ok(scalar keys %signo > 1 && exists $signo{KILL}, '$Config{sig_name} parsed correctly');
+  die q|A child proc wasn't killed and did cleanup on its own| if ! -e 'killchildstarted';
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  #no END block, done earlier
+  $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  is(kill(-$signo{KILL}, $pid), 1, 'process group kill, numeric signal');
+}
diff --git a/t/op/kill0_child b/t/op/kill0_child
new file mode 100644
index 0000000..c3d5eb2
--- /dev/null
+++ b/t/op/kill0_child
@@ -0,0 +1,9 @@
+#$ARGV[0] is filename used to notify parent .t perl proc that all PIDs are
+#started in the process tree
+#number 9999/9998 is eye catching
+system(1, $^X, '-e', 'sleep 5; print qq|not ok 9999 - inner child process wasn\'t killed\n|;');
+system('echo outer child started > "'.$ARGV[0].'"');
+sleep 5;
+#execution won't be reached if test successful
+print "not ok 9998 - outer child process wasn\'t killed\n";
+unlink($ARGV[0]);
-- 
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 27, 2014

From @Hugmeir

On Thu, Feb 27, 2014 at 10​:07 AM, bulk88 via RT
<perlbug-followup@​perl.org>wrote​:

On Wed Feb 26 23​:06​:03 2014, bulk88 wrote​:

This patch is broken. Fails manifest.t and I forgot perldelta.pod. I
have manifest.t fixed, but I am having a perldelta.pod and podcheck.t
problem that is being addressed in a separate ML thread. So wait until
I have a new patch.

New patch attached.

--
bulk88 ~ bulk88 at hotmail.com

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121230

From dc146462729d13b3038d9a58ac6291bc2e09f77f Mon Sep 17 00​:00​:00 2001
From​: bulk88 <bulk88@​hotmail.com>
Date​: Thu, 27 Feb 2014 04​:00​:43 -0500
Subject​: [PATCH] RT #121230, create tests for process group kill on Win32

---
MANIFEST | 3 ++-
pod/perldelta.pod | 9 +++++++--
t/op/kill0.t | 41 ++++++++++++++++++++++++++++++++++++++++-
t/op/kill0_child | 9 +++++++++
4 files changed, 58 insertions(+), 4 deletions(-)
create mode 100644 t/op/kill0_child

diff --git a/MANIFEST b/MANIFEST
index 9fcb518..e7ab59a 100644
--- a/MANIFEST
+++ b/MANIFEST
@​@​ -5238,7 +5238,8 @​@​ t/op/index.t See if index works
t/op/index_thr.t See if index works in another thread
t/op/int.t See if int works
t/op/join.t See if join works
-t/op/kill0.t See if kill(0, $pid) works
+t/op/kill0_child Process tree script that is kill()ed
+t/op/kill0.t See if kill works
t/op/kvaslice.t See if index/value array slices
work
t/op/kvhslice.t See if key/value hash slices work
t/op/lc.t See if lc, uc, lcfirst, ucfirst, quotemeta
work
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index ab26873..f298d47 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@​@​ -319,11 +319,16 @​@​ and compilation changes or changes in
portability/compatibility. However,
changes within modules for platforms should generally be listed in the
L</Modules and Pragmata> section.

+=head3 Win32
+
=over 4

-=item XXX-some-platform
+=item *

-XXX
+Killing a process tree with L<perlfunc/kill> and a negative signal, was
broken
+starting in 5.17.2. In this bug, C<kill> always returned 0 for a negative
+signal even for valid PIDs, and no processes were terminated. This has
been
+fixed [perl #121230].

Better to use 5.18.0 here -- the vast majority of users have no reason to
care about dev releases.

=back

diff --git a/t/op/kill0.t b/t/op/kill0.t
index d3ef8f7..4012761 100644
--- a/t/op/kill0.t
+++ b/t/op/kill0.t
@​@​ -13,8 +13,9 @​@​ BEGIN {
}

use strict;
+use Config;

-plan tests => 6;
+plan tests => 9;

ok( kill(0, $$), 'kill(0, $pid) returns true if $pid exists' );

@​@​ -50,3 +51,41 @​@​ for my $case ( @​bad_pids ) {
$x =~ /(\d+)/;
ok(eval { kill 0, $1 }, "can kill a number string in a magic variable");
}
+
+SKIP​: {
+ skip 'custom process group kill() only on Win32', 3 if ($^O ne
'MSWin32');
+ #create 2 child processes, an outer one created by kill0.t, and an
inner one
+ #created by outer this allows the test to fail if only the outer one was
+ #killed, since the inner will stay around and eventually print failed
and
+ #out of sequence TAP to harness
+ unlink('killchildstarted');
+ die q|can't unlink| if -e 'killchildstarted';
+ eval q|END{unlink('killchildstarted');}|;
+ my $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+ die 'PID is 0' if !$pid;
+ while( ! -e 'killchildstarted') {
+ sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+ }
+ #ways to break this test manually, change '-KILL' to 'KILL', change
$pid to a
+ #bogus number
+ is(kill('-KILL', $pid), 1, 'process group kill, named signal');
+
+ my ($i, %signo, @​signame, $sig_name) = 0;
+ ($sig_name = $Config{sig_name}) || die "No signals?";
+ foreach my $name (split(' ', $sig_name)) {
+ $signo{$name} = $i;
+ $signame[$i] = $name;
+ $i++;
+ }
+ ok(scalar keys %signo > 1 && exists $signo{KILL}, '$Config{sig_name}
parsed correctly');
+ die q|A child proc wasn't killed and did cleanup on its own| if ! -e
'killchildstarted';
+ unlink('killchildstarted');
+ die q|can't unlink| if -e 'killchildstarted';
+ #no END block, done earlier
+ $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+ die 'PID is 0' if !$pid;
+ while( ! -e 'killchildstarted') {
+ sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+ }
+ is(kill(-$signo{KILL}, $pid), 1, 'process group kill, numeric signal');
+}
diff --git a/t/op/kill0_child b/t/op/kill0_child
new file mode 100644
index 0000000..c3d5eb2
--- /dev/null
+++ b/t/op/kill0_child
@​@​ -0,0 +1,9 @​@​
+#$ARGV[0] is filename used to notify parent .t perl proc that all PIDs are
+#started in the process tree
+#number 9999/9998 is eye catching
+system(1, $^X, '-e', 'sleep 5; print qq|not ok 9999 - inner child process
wasn\'t killed\n|;');
+system('echo outer child started > "'.$ARGV[0].'"');
+sleep 5;

^ how thoroughly was this tested? <10 second waits tend to fail on VMs
quite often.

+#execution won't be reached if test successful
+print "not ok 9998 - outer child process wasn\'t killed\n";
+unlink($ARGV[0]);
--
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 27, 2014

From @bulk88

On Thu Feb 27 03​:01​:28 2014, Hugmeir wrote​:

On Thu, Feb 27, 2014 at 10​:07 AM, bulk88 via RT

New patch attached.

--
bulk88 ~ bulk88 at hotmail.com

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121230

From dc146462729d13b3038d9a58ac6291bc2e09f77f Mon Sep 17 00​:00​:00
2001
From​: bulk88 <bulk88@​hotmail.com>
Date​: Thu, 27 Feb 2014 04​:00​:43 -0500
Subject​: [PATCH] RT #121230, create tests for process group kill on
Win32

---
MANIFEST | 3 ++-
pod/perldelta.pod | 9 +++++++--
t/op/kill0.t | 41 ++++++++++++++++++++++++++++++++++++++++-
t/op/kill0_child | 9 +++++++++
4 files changed, 58 insertions(+), 4 deletions(-)
create mode 100644 t/op/kill0_child

diff --git a/MANIFEST b/MANIFEST
index 9fcb518..e7ab59a 100644
--- a/MANIFEST
+++ b/MANIFEST
@​@​ -5238,7 +5238,8 @​@​ t/op/index.t See if index
works
t/op/index_thr.t See if index works in another thread
t/op/int.t See if int works
t/op/join.t See if join works
-t/op/kill0.t See if kill(0, $pid) works
+t/op/kill0_child Process tree script that is kill()ed
+t/op/kill0.t See if kill works
t/op/kvaslice.t See if index/value array
slices
work
t/op/kvhslice.t See if key/value hash slices
work
t/op/lc.t See if lc, uc, lcfirst, ucfirst,
quotemeta
work
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index ab26873..f298d47 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@​@​ -319,11 +319,16 @​@​ and compilation changes or changes in
portability/compatibility. However,
changes within modules for platforms should generally be listed in
the
L</Modules and Pragmata> section.

+=head3 Win32
+
=over 4

-=item XXX-some-platform
+=item *

-XXX
+Killing a process tree with L<perlfunc/kill> and a negative signal,
was
broken
+starting in 5.17.2. In this bug, C<kill> always returned 0 for a
negative
+signal even for valid PIDs, and no processes were terminated. This
has
been
+fixed [perl #121230].

Better to use 5.18.0 here -- the vast majority of users have no reason
to
care about dev releases.

Will revise. But I need a response to my comments about the next comment first.

=back

diff --git a/t/op/kill0.t b/t/op/kill0.t
index d3ef8f7..4012761 100644
--- a/t/op/kill0.t
+++ b/t/op/kill0.t
@​@​ -13,8 +13,9 @​@​ BEGIN {
}

use strict;
+use Config;

-plan tests => 6;
+plan tests => 9;

ok( kill(0, $$), 'kill(0, $pid) returns true if $pid exists' );

@​@​ -50,3 +51,41 @​@​ for my $case ( @​bad_pids ) {
$x =~ /(\d+)/;
ok(eval { kill 0, $1 }, "can kill a number string in a magic
variable");
}
+
+SKIP​: {
+ skip 'custom process group kill() only on Win32', 3 if ($^O ne
'MSWin32');
+ #create 2 child processes, an outer one created by kill0.t, and an
inner one
+ #created by outer this allows the test to fail if only the outer
one was
+ #killed, since the inner will stay around and eventually print
failed
and
+ #out of sequence TAP to harness
+ unlink('killchildstarted');
+ die q|can't unlink| if -e 'killchildstarted';
+ eval q|END{unlink('killchildstarted');}|;
+ my $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+ die 'PID is 0' if !$pid;
+ while( ! -e 'killchildstarted') {
+ sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+ }
+ #ways to break this test manually, change '-KILL' to 'KILL',
change
$pid to a
+ #bogus number
+ is(kill('-KILL', $pid), 1, 'process group kill, named signal');
+
+ my ($i, %signo, @​signame, $sig_name) = 0;
+ ($sig_name = $Config{sig_name}) || die "No signals?";
+ foreach my $name (split(' ', $sig_name)) {
+ $signo{$name} = $i;
+ $signame[$i] = $name;
+ $i++;
+ }
+ ok(scalar keys %signo > 1 && exists $signo{KILL},
'$Config{sig_name}
parsed correctly');
+ die q|A child proc wasn't killed and did cleanup on its own| if !
-e
'killchildstarted';
+ unlink('killchildstarted');
+ die q|can't unlink| if -e 'killchildstarted';
+ #no END block, done earlier
+ $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+ die 'PID is 0' if !$pid;
+ while( ! -e 'killchildstarted') {
+ sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+ }
+ is(kill(-$signo{KILL}, $pid), 1, 'process group kill, numeric
signal');
+}
diff --git a/t/op/kill0_child b/t/op/kill0_child
new file mode 100644
index 0000000..c3d5eb2
--- /dev/null
+++ b/t/op/kill0_child
@​@​ -0,0 +1,9 @​@​
+#$ARGV[0] is filename used to notify parent .t perl proc that all
PIDs are
+#started in the process tree
+#number 9999/9998 is eye catching
+system(1, $^X, '-e', 'sleep 5; print qq|not ok 9999 - inner child
process
wasn\'t killed\n|;');
+system('echo outer child started > "'.$ARGV[0].'"');
+sleep 5;

^ how thoroughly was this tested? <10 second waits tend to fail on VMs
quite often.

Ran it a couple dozen times repeatedly breaking it by giving bogus signals, + signals, and fake pids and made sure harness failed as a not ok fail or out of sequence fail. Originally kill0.t did a "sleep 1;" to wait for the outer and inner procs to start. You dont want to kill the outer before it starts the inner proc which is what I encountered if the kill was sent with no delay after the system(). I then realized this is begging for a race condition and subsequent test fail in a VM with rough host timeslicing. I then switched to using the disk file. Win32​::Event isn't core http​://search.cpan.org/~cjm/Win32-IPC-1.09/lib/Win32/Event.pm . So I can't use that as IPC. It also is XS.

Sleeping a very high number near infinity sounds like a bad idea to me and a way to hang a smoker if something goes wrong (and what will go wrong can't be predicted). I picked 5 secs because it seems like a good balance between waiting the kill0.t to get back its timeslice and do the kill, vs I/O blocking harness since kill0.t failed to kill the 2 child procs (future or manual breakage), and now harness is waiting 4 seconds until the 2 child procs close their stdio handles, and probably (assuming a lack of random kernel memory corruption and being a couple ms away from a panic/bsod) will write "not ok" to the tap stream. Also I picked 5 instead of 3 secs because of a possibility of future parallel testing on Win32.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 28, 2014

From @iabyn

On Sun, Feb 23, 2014 at 01​:57​:23PM +0000, Dave Mitchell wrote​:

I've pushed a fix as smoke-me/davem/killpg. I haven't of course tested it
on Win32. If you're able to confirm it works, and if the smokes are ok,
I'll merge it into blead. It should then be a candidate for maint-5.18.

Now in blead as

111f73b

--
Indomitable in retreat, invincible in advance, insufferable in victory
  -- Churchill on Montgomery

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2014

From @bulk88

On Thu Feb 27 09​:02​:49 2014, bulk88 wrote​:

On Thu Feb 27 03​:01​:28 2014, Hugmeir wrote​:

Better to use 5.18.0 here -- the vast majority of users have no
reason
to
care about dev releases.

Will revise. But I need a response to my comments about the next
comment first.

I don't think I will get a response. So it stays at 5 seconds and a new patch mentioning 5.18.0 instead of 5.17.2 is attached.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2014

From @bulk88

0001-RT-121230-create-tests-for-process-group-kill-on-Win.patch
From d37c11f7b2b728856f89ce7f8afc7665f6922d8a Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Fri, 28 Feb 2014 23:56:54 -0500
Subject: [PATCH] RT #121230, create tests for process group kill on Win32

---
 MANIFEST          |    3 ++-
 pod/perldelta.pod |    9 +++++++--
 t/op/kill0.t      |   41 ++++++++++++++++++++++++++++++++++++++++-
 t/op/kill0_child  |    9 +++++++++
 4 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100644 t/op/kill0_child

diff --git a/MANIFEST b/MANIFEST
index 9fcb518..e7ab59a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5238,7 +5238,8 @@ t/op/index.t			See if index works
 t/op/index_thr.t		See if index works in another thread
 t/op/int.t			See if int works
 t/op/join.t			See if join works
-t/op/kill0.t			See if kill(0, $pid) works
+t/op/kill0_child		Process tree script that is kill()ed
+t/op/kill0.t			See if kill works
 t/op/kvaslice.t			See if index/value array slices work
 t/op/kvhslice.t			See if key/value hash slices work
 t/op/lc.t			See if lc, uc, lcfirst, ucfirst, quotemeta work
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index ab26873..1f413ff 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -319,11 +319,16 @@ and compilation changes or changes in portability/compatibility.  However,
 changes within modules for platforms should generally be listed in the
 L</Modules and Pragmata> section.
 
+=head3 Win32
+
 =over 4
 
-=item XXX-some-platform
+=item *
 
-XXX
+Killing a process tree with L<perlfunc/kill> and a negative signal, was broken
+starting in 5.18.0. In this bug, C<kill> always returned 0 for a negative
+signal even for valid PIDs, and no processes were terminated. This has been
+fixed [perl #121230].
 
 =back
 
diff --git a/t/op/kill0.t b/t/op/kill0.t
index d3ef8f7..4012761 100644
--- a/t/op/kill0.t
+++ b/t/op/kill0.t
@@ -13,8 +13,9 @@ BEGIN {
 }
 
 use strict;
+use Config;
 
-plan tests => 6;
+plan tests => 9;
 
 ok( kill(0, $$), 'kill(0, $pid) returns true if $pid exists' );
 
@@ -50,3 +51,41 @@ for my $case ( @bad_pids ) {
   $x =~ /(\d+)/;
   ok(eval { kill 0, $1 }, "can kill a number string in a magic variable");
 }
+
+SKIP: {
+  skip 'custom process group kill() only on Win32', 3 if ($^O ne 'MSWin32');
+  #create 2 child processes, an outer one created by kill0.t, and an inner one
+  #created by outer this allows the test to fail if only the outer one was
+  #killed, since the inner will stay around and eventually print failed and
+  #out of sequence TAP to harness
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  eval q|END{unlink('killchildstarted');}|;
+  my $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  #ways to break this test manually, change '-KILL' to 'KILL', change $pid to a
+  #bogus number
+  is(kill('-KILL', $pid), 1, 'process group kill, named signal');
+
+  my ($i, %signo, @signame, $sig_name) = 0;
+  ($sig_name = $Config{sig_name}) || die "No signals?";
+  foreach my $name (split(' ', $sig_name)) {
+    $signo{$name} = $i;
+    $signame[$i] = $name;
+    $i++;
+  }
+  ok(scalar keys %signo > 1 && exists $signo{KILL}, '$Config{sig_name} parsed correctly');
+  die q|A child proc wasn't killed and did cleanup on its own| if ! -e 'killchildstarted';
+  unlink('killchildstarted');
+  die q|can't unlink| if -e 'killchildstarted';
+  #no END block, done earlier
+  $pid = system(1, $^X, 'op/kill0_child', 'killchildstarted');
+  die 'PID is 0' if !$pid;
+  while( ! -e 'killchildstarted') {
+    sleep 1; #a sleep 0 with $i++ will takes ~160 iterations here
+  }
+  is(kill(-$signo{KILL}, $pid), 1, 'process group kill, numeric signal');
+}
diff --git a/t/op/kill0_child b/t/op/kill0_child
new file mode 100644
index 0000000..c3d5eb2
--- /dev/null
+++ b/t/op/kill0_child
@@ -0,0 +1,9 @@
+#$ARGV[0] is filename used to notify parent .t perl proc that all PIDs are
+#started in the process tree
+#number 9999/9998 is eye catching
+system(1, $^X, '-e', 'sleep 5; print qq|not ok 9999 - inner child process wasn\'t killed\n|;');
+system('echo outer child started > "'.$ARGV[0].'"');
+sleep 5;
+#execution won't be reached if test successful
+print "not ok 9998 - outer child process wasn\'t killed\n";
+unlink($ARGV[0]);
-- 
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2014

From @bulk88

On Fri Feb 28 20​:59​:15 2014, bulk88 wrote​:

I don't think I will get a response. So it stays at 5 seconds and a
new patch mentioning 5.18.0 instead of 5.17.2 is attached.

Bump.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2014

From @iabyn

On Fri, Feb 28, 2014 at 08​:59​:15PM -0800, bulk88 via RT wrote​:

I don't think I will get a response. So it stays at 5 seconds and a new
patch mentioning 5.18.0 instead of 5.17.2 is attached.

Thanks, applied as af728ca. I also took the libery of cleaning the code
up a bit with the following commit​:

commit 4c0e595
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Mar 17 16​:19​:10 2014 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Mar 17 16​:35​:07 2014 +0000

  tidy up kill0.t and kill0_child
 
  The previous commit added some tests to kill0.t, and added the auxiliary
  file kill0_child. Tidy up the new code to better match normal standards.
  In particular, improve the format, grammar and clarity of the comments,
  and replace q|...| with "..." where appropriate.
  Also, make the temporary filename a variable, and prefix it with "tmp-",
  so that if gets left around for any reason, it's more obvious that it's
  just an extraneous temporary file.
 
  (I haven't actually tested this commit on win32)

--
"Strange women lying in ponds distributing swords is no basis for a system
of government. Supreme executive power derives from a mandate from the
masses, not from some farcical aquatic ceremony."
  -- Dennis, "Monty Python and the Holy Grail"

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2014

From @iabyn

On Wed, Feb 26, 2014 at 03​:16​:25PM -0800, bulk88 via RT wrote​:

I think the perl docs are misleading. It's not supposed to indicate
the total *processes* killed - UNIX certainly doesn't provide this info
when killing groups - but rather the number of 'kills' in the list
that were successful. i.e.

kill $SIG\, $proc1\, $proc2\, $no\_such\_process;

returns 2.

Patch for POD.

Sends a signal to a list of processes. Returns the number of
processes successfully signaled (which is not necessarily the
-same as the number actually killed).
+same as the number actually killed) from the list of processes supplied.

Thanks. However, I ended up rewording it again as follows, with 12733a0,
since I think the process / process group distinction still wasn't clear
enough.

-Sends a signal to a list of processes. Returns the number of
-processes successfully signaled (which is not necessarily the
-same as the number actually killed).
+Sends a signal to a list of processes. Returns the number of arguments
+that were successfully used to signal (which is not necessarily the same
+as the number of processes actually killed, e.g. where a process group is
+killed).

--
In England there is a special word which means the last sunshine
of the summer. That word is "spring".

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2014

From @bulk88

On Mon Mar 17 09​:40​:43 2014, davem wrote​:

Thanks, applied as af728ca. I also took the libery of cleaning the code
up a bit with the following commit​:

commit 4c0e595
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Mar 17 16​:19​:10 2014 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Mar 17 16​:35​:07 2014 +0000

tidy up kill0\.t and kill0\_child

The previous commit added some tests to kill0\.t\, and added the auxiliary
file kill0\_child\. Tidy up the new code to better match normal standards\.
In particular\, improve the format\, grammar and clarity of the comments\,
and replace q|\.\.\.| with "\.\.\." where appropriate\.
Also\, make the temporary filename a variable\, and prefix it with "tmp\-"\,
so that if gets left around for any reason\, it's more obvious that it's
just an extraneous temporary file\.

Everything is fine in that except 1 thing.

Why replace the single quotes with double quotes for the sake of using double quotes? I've always assume double quotes take longer for the compiler to parse than single quotes, and using double quotes without knowing why leads to a larger change of unintended code execution/interpolation.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 18, 2014

From @iabyn

On Mon, Mar 17, 2014 at 02​:43​:56PM -0700, bulk88 via RT wrote​:

Everything is fine in that except 1 thing.

Why replace the single quotes with double quotes for the sake of using
double quotes? I've always assume double quotes take longer for the
compiler to parse than single quotes, and using double quotes without
knowing why leads to a larger change of unintended code
execution/interpolation.

The difference in parsing time between single and double quotes strings is
infinitesimal and should not be a consideration in a small test file,
especially one that does a lot of forking and sleeping.

So I went for readability. IMO,

  "that's all"
 
is much easier for the brain to parse than

  q|that's all|

since the '|' character in perl isn't normally associated with string
delimiting.

The choice of single verses double-quoted is debatable. It can cause
errors either way. For example someone later modifying your version might
accidentally add​:

  q|that's all, $person|

while someone modifying my code might do

  "that's all you get for $100"

In either case, if the programmer doesn't check the quotedness of string
before modifying it, they get everything they deserve.

--
A power surge on the Bridge is rapidly and correctly diagnosed as a faulty
capacitor by the highly-trained and competent engineering staff.
  -- Things That Never Happen in "Star Trek" #9

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 18, 2014

From @demerphq

On 18 March 2014 13​:03, Dave Mitchell <davem@​iabyn.com> wrote​:

On Mon, Mar 17, 2014 at 02​:43​:56PM -0700, bulk88 via RT wrote​:

Everything is fine in that except 1 thing.

Why replace the single quotes with double quotes for the sake of using
double quotes? I've always assume double quotes take longer for the
compiler to parse than single quotes, and using double quotes without
knowing why leads to a larger change of unintended code
execution/interpolation.

The difference in parsing time between single and double quotes strings is
infinitesimal

IMO there is no point in worrying about these kind of things unless
the string is very long.

Yves

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 18, 2014

From @Tux

On Tue, 18 Mar 2014 13​:15​:11 +0100, demerphq <demerphq@​gmail.com> wrote​:

The difference in parsing time between single and double quotes strings is
infinitesimal

IMO there is no point in worrying about these kind of things unless
the string is very long.

And not even then. I benched it years ago on several machines, and all
bench results are in line-noise margins and differ per architecture.
IIRC all big-endian boxes I tried on had a <1% preference for double
quotes whereas little-endian boxes prefered single quotes.

Let's just be consistent!

More interesting would be to check if "foo​: ".$foo.", bar​: ".$bar is
equally fast as "foo​: $foo, bar​: $bar". Last time I checked, the
interpolated version was faster

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 21, 2014

From @bulk88

On Mon Mar 17 09​:40​:43 2014, davem wrote​:

On Fri, Feb 28, 2014 at 08​:59​:15PM -0800, bulk88 via RT wrote​:

I don't think I will get a response. So it stays at 5 seconds and a new
patch mentioning 5.18.0 instead of 5.17.2 is attached.

Thanks, applied as af728ca. I also took the libery of cleaning the code
up a bit with the following commit​:

commit 4c0e595
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Mar 17 16​:19​:10 2014 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Mar 17 16​:35​:07 2014 +0000

tidy up kill0\.t and kill0\_child

The previous commit added some tests to kill0\.t\, and added the auxiliary
file kill0\_child\. Tidy up the new code to better match normal standards\.
In particular\, improve the format\, grammar and clarity of the comments\,
and replace q|\.\.\.| with "\.\.\." where appropriate\.
Also\, make the temporary filename a variable\, and prefix it with "tmp\-"\,
so that if gets left around for any reason\, it's more obvious that it's
just an extraneous temporary file\.

\(I haven't actually tested this commit on win32\)

http​://www.nntp.perl.org/group/perl.daily-build.reports/2014/03/msg159824.html the test is failing on Geroge smoker. I fetched a new blead and reran it on my machine and ../t/op/kill0.t passes. The kill() calls fail. What is unusual is that there is no "not ok 9999" or out of sequence reports in the report, so I presume the child procs never got CPU time even though they got PIDs or were actually killed on an OS level and something went wrong in the win32_killpg. The only idea I have right now is to increase the sleep 5 to 15 in http​://perl5.git.perl.org/perl.git/blob/HEAD​:/t/op/kill0_child .

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 22, 2014

From @bulk88

On Fri Mar 21 13​:58​:09 2014, bulk88 wrote​:

http​://www.nntp.perl.org/group/perl.daily-
build.reports/2014/03/msg159824.html the test is failing on Geroge
smoker. I fetched a new blead and reran it on my machine and
../t/op/kill0.t passes. The kill() calls fail. What is unusual is that
there is no "not ok 9999" or out of sequence reports in the report, so
I presume the child procs never got CPU time even though they got PIDs
or were actually killed on an OS level and something went wrong in the
win32_killpg. The only idea I have right now is to increase the sleep
5 to 15 in
http​://perl5.git.perl.org/perl.git/blob/HEAD​:/t/op/kill0_child .

I sort of (no CC no make, tests that use either fail catasfrical (not a word, i kno), M​::B hangs forever on nearly every test) reran blead on a win2k machine. nothing kill related failed and the kill .ts didnt fail. So its something specific to a VM or the george software stack.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2014

From @rjbs

bulk88 requested that this be marked a 5.20.0 blocker, but I'm not entirely sure what issue remains on the ticket, on which discussion has sort of meandered.

(a) is this still a blocker candidate?
(b) what's up?

--
rjbs

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2014

From @iabyn

On Mon, Mar 24, 2014 at 07​:19​:37AM -0700, Ricardo SIGNES via RT wrote​:

bulk88 requested that this be marked a 5.20.0 blocker, but I'm not entirely sure what issue remains on the ticket, on which discussion has sort of meandered.

(a) is this still a blocker candidate?
(b) what's up?

The bug has been fixed, but the just-added test is failing on George's
smoker, but not for bulk88.

So we either need to get the test fixed, or backout the new test.

--
More than any other time in history, mankind faces a crossroads. One path
leads to despair and utter hopelessness. The other, to total extinction.
Let us pray we have the wisdom to choose correctly.
  -- Woody Allen

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2014

From @bulk88

Dave Mitchell wrote​:

On Mon, Mar 24, 2014 at 07​:19​:37AM -0700, Ricardo SIGNES via RT wrote​:

bulk88 requested that this be marked a 5.20.0 blocker, but I'm not entirely sure what issue remains on the ticket, on which discussion has sort of meandered.

(a) is this still a blocker candidate?
(b) what's up?

The bug has been fixed, but the just-added test is failing on George's
smoker, but not for bulk88.

So we either need to get the test fixed, or backout the new test.

5.18 maint backport is also a possibility since this is a regression for
a feature nobody knew existed, except for probably demerphq (who wrote
IIRC) and he doesnt use Windows anymore. But first figuring out why the
George smoker failed is required. Steve Hay, Tony, can you 2 trying
building Win32 Perl and see what happens? There arent any active Win32
porters other than me, Hay and Cook AFAIK.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2014

From @steve-m-hay

On 24 March 2014 21​:58, bulk88 <bulk88@​hotmail.com> wrote​:

Dave Mitchell wrote​:

On Mon, Mar 24, 2014 at 07​:19​:37AM -0700, Ricardo SIGNES via RT wrote​:

bulk88 requested that this be marked a 5.20.0 blocker, but I'm not
entirely sure what issue remains on the ticket, on which discussion has sort
of meandered.

(a) is this still a blocker candidate?
(b) what's up?

The bug has been fixed, but the just-added test is failing on George's
smoker, but not for bulk88.

So we either need to get the test fixed, or backout the new test.

5.18 maint backport is also a possibility since this is a regression for a
feature nobody knew existed, except for probably demerphq (who wrote IIRC)
and he doesnt use Windows anymore. But first figuring out why the George
smoker failed is required. Steve Hay, Tony, can you 2 trying building Win32
Perl and see what happens? There arent any active Win32 porters other than
me, Hay and Cook AFAIK.

Sorry, I had assumed that I couldn't reproduce either this since my
recent perl build/tests have all been happily passing, but on closer
inspection I see that George's smoker only fails without useithreads.

So I built with USE_MULTI=undef USE_ITHREADS=undef USE_IMP_SYS=undef
and sure enough it fails​:

C​:\Dev\Git\perl\t>..\perl harness -v op\kill0.t
op/kill0.t ..
1..9
ok 1 - kill(0, $pid) returns true if $pid exists
ok 2 - kill(0, $pid) returns false if $pid does not exist
ok 3 - dies killing undef pid
ok 4 - dies killing empty string pid
ok 5 - dies killing alphabetic pid
ok 6 - can kill a number string in a magic variable
# Failed test 7 - process group kill, named signal at op/kill0.t line 82
# got "0"
# expected "1"
not ok 7 - process group kill, named signal
ok 8 - $Config{sig_name} parsed correctly
# Failed test 9 - process group kill, numeric signal at op/kill0.t line 107
# got "0"
# expected "1"
not ok 9 - process group kill, numeric signal
Failed 2/9 subtests

Test Summary Report


op/kill0.t (Wstat​: 0 Tests​: 9 Failed​: 2)
  Failed tests​: 7, 9
Files=1, Tests=9, 2 wallclock secs ( 0.05 usr + 0.00 sys = 0.05 CPU)
Result​: FAIL

I don't have time to look right now, but will dig deeper tomorrow...

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 25, 2014

From @bulk88

On Mon Mar 24 16​:09​:44 2014, shay wrote​:

Sorry, I had assumed that I couldn't reproduce either this since my
recent perl build/tests have all been happily passing, but on closer
inspection I see that George's smoker only fails without useithreads.

So I built with USE_MULTI=undef USE_ITHREADS=undef USE_IMP_SYS=undef
and sure enough it fails​:

Reproduced.


C​:\perl519\src\t>..\perl -I..\lib op/kill0.t
1..9
ok 1 - kill(0, $pid) returns true if $pid exists
ok 2 - kill(0, $pid) returns false if $pid does not exist
ok 3 - dies killing undef pid
ok 4 - dies killing empty string pid
ok 5 - dies killing alphabetic pid
ok 6 - can kill a number string in a magic variable
not ok 7 - process group kill, named signal
# Failed test 7 - process group kill, named signal at op/kill0.t line 82
# got "0"
# expected "1"
ok 8 - $Config{sig_name} parsed correctly
not ok 9 - process group kill, numeric signal
# Failed test 9 - process group kill, numeric signal at op/kill0.t line 107
# got "0"
# expected "1"

C​:\perl519\src\t>


I will investigate.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 25, 2014

From @bulk88

On Mon Mar 24 17​:11​:39 2014, bulk88 wrote​:

I will investigate.

Perl's win32 killpg in win32.c doesn't match the POSIX expectations that Perl_apply has for killpg. When PERL_IMPLICIT_SYS/perlhost.h/PerlProcKillpg is removed, then Perl_apply directly calls killpg which follows Win32 coding style, not POSIX coding style. With PERL_IMPLICIT_SYS, PerlProcKillpg calls win32_kill which calls my_kill which calls killpg. I plan on renaming killpg to my_killpg, then create a win32_killpg that follows POSIXese, and untangle the spaghetti.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 26, 2014

From @bulk88

On Mon Mar 24 18​:00​:26 2014, bulk88 wrote​:

I plan on renaming
killpg to my_killpg, then create a win32_killpg that follows POSIXese,
and untangle the spaghetti.

Code already written but I can't make test it ATM. Will take a day or 2 to get to posting the patch, need to file another Perl RT or CPAN RT bug first since make test fails badly for me due to a /cpan module.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 27, 2014

From @bulk88

On Tue Mar 25 22​:52​:13 2014, bulk88 wrote​:

On Mon Mar 24 18​:00​:26 2014, bulk88 wrote​:

I plan on renaming
killpg to my_killpg, then create a win32_killpg that follows
POSIXese,
and untangle the spaghetti.

Code already written but I can't make test it ATM. Will take a day or
2 to get to posting the patch, need to file another Perl RT or CPAN RT
bug first since make test fails badly for me due to a /cpan module.

kill0.t passed for me on no-threads perl with this patch. I couldn't make harness finish due to a different bug.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 27, 2014

From @bulk88

0001-fix-killpg-on-Win32-to-meet-posix-expectations-for-k.patch
From 5916dd7a8033a4d89aa7f6d7e52a718e96a7fa4d Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 27 Mar 2014 03:37:03 -0400
Subject: [PATCH] fix killpg on Win32, to meet posix expectations for killpg

On Win32 Perls built without PERL_IMPLICIT_SYS, killpg from win32.c was
directly called by Perl_apply, yet killpg's return value had Win32
behavior, not POSIX behavior. Modify killpg token to have same meaning as
PerlProcKillpg/PerlProc_killpg has on PERL_IMPLICIT_SYS builds. Use a
macro rather than create a win32_killpg C function since win32_killpg would
be nothing but a call to win32_kill anyways. win32_kill contains the Win32
to POSIX semantics conversion code. Rename old killpg to my_killpg since
it has no use outside of win32.c. The psuedo-PID code in win32_kill also
played a factor in not writing a separate win32_killpg that calls
my_killpg. This fix is tested by kill0.t passing on
no-PERL_IMPLICIT_SYS builds.

[perl #121230]
---
 pod/perldelta.pod |    8 ++++++++
 win32/win32.c     |   11 +++++++----
 win32/win32.h     |    1 -
 win32/win32iop.h  |    5 +++++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index 358aea3..aa7e1ce 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -316,6 +316,14 @@ a bug in which the timeout monitor used for tests could not be cancelled once
 the test completes, and the full timeout period elapsed before running the next
 test file.
 
+On a Perl built without psuedo-fork (psuedo-fork builds were not affected by
+this bug), probably since prcess tree kill feature was implemented on Win32,
+killing a process tree with L<perlfunc/kill> and a negative signal, resulted
+in kill inverting the returned value.  This ment successfully killing
+1 process tree PID returned 0, and also passing 2 invalid PID, returned 2.
+This has been corrected so the documented behavior for return values for kill
+executes.  [perl #121230]
+
 =back
 
 =head1 Internal Changes
diff --git a/win32/win32.c b/win32/win32.c
index dda951b..6dc6ec1 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -139,6 +139,7 @@ static int	do_spawnvp_handles(int mode, const char *cmdname,
 static long	find_pid(pTHX_ int pid);
 static void	remove_dead_process(long child);
 static int	terminate_process(DWORD pid, HANDLE process_handle, int sig);
+static int	my_killpg(int pid, int sig);
 static int	my_kill(int pid, int sig);
 static void	out_of_memory(void);
 static char*	wstr_to_str(const wchar_t* wstr);
@@ -1250,8 +1251,9 @@ terminate_process(DWORD pid, HANDLE process_handle, int sig)
     return 0;
 }
 
-int
-killpg(int pid, int sig)
+/* returns number of processes killed */
+static int
+my_killpg(int pid, int sig)
 {
     HANDLE process_handle;
     HANDLE snapshot_handle;
@@ -1271,7 +1273,7 @@ killpg(int pid, int sig)
         if (Process32First(snapshot_handle, &entry)) {
             do {
                 if (entry.th32ParentProcessID == (DWORD)pid)
-                    killed += killpg(entry.th32ProcessID, sig);
+                    killed += my_killpg(entry.th32ProcessID, sig);
                 entry.dwSize = sizeof(entry);
             }
             while (Process32Next(snapshot_handle, &entry));
@@ -1282,6 +1284,7 @@ killpg(int pid, int sig)
     return killed;
 }
 
+/* returns number of processes killed */
 static int
 my_kill(int pid, int sig)
 {
@@ -1289,7 +1292,7 @@ my_kill(int pid, int sig)
     HANDLE process_handle;
 
     if (sig < 0)
-        return killpg(pid, -sig);
+        return my_killpg(pid, -sig);
 
     process_handle = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
     /* OpenProcess() returns NULL on error, *not* INVALID_HANDLE_VALUE */
diff --git a/win32/win32.h b/win32/win32.h
index 3d1655a..e109939 100644
--- a/win32/win32.h
+++ b/win32/win32.h
@@ -322,7 +322,6 @@ extern  gid_t	getegid(void);
 extern  int	setuid(uid_t uid);
 extern  int	setgid(gid_t gid);
 extern  int	kill(int pid, int sig);
-extern  int	killpg(int pid, int sig);
 #ifndef USE_PERL_SBRK
 extern  void	*sbrk(ptrdiff_t need);
 #  define HAS_SBRK_PROTO
diff --git a/win32/win32iop.h b/win32/win32iop.h
index 11d4219..246375f 100644
--- a/win32/win32iop.h
+++ b/win32/win32iop.h
@@ -448,6 +448,11 @@ END_EXTERN_C
 #  undef kill
 #endif
 #define kill			win32_kill
+#ifdef UNDER_CE
+#  undef killpg
+#endif
+#define killpg(pid, sig)	win32_kill(pid, -(sig))
+
 
 #ifdef UNDER_CE
 #  undef opendir
-- 
1.7.9.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 27, 2014

From @steve-m-hay

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Tue Mar 25 22​:52​:13 2014, bulk88 wrote​:

On Mon Mar 24 18​:00​:26 2014, bulk88 wrote​:

I plan on renaming
killpg to my_killpg, then create a win32_killpg that follows
POSIXese,
and untangle the spaghetti.

Code already written but I can't make test it ATM. Will take a day or
2 to get to posting the patch, need to file another Perl RT or CPAN RT
bug first since make test fails badly for me due to a /cpan module.

kill0.t passed for me on no-threads perl with this patch. I couldn't make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed now?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 27, 2014

From @bulk88

On Thu Mar 27 06​:01​:15 2014, shay wrote​:

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org>

kill0.t passed for me on no-threads perl with this patch. I couldn't
make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed now?

Lets wait until the George smoker delivers a new report. I see nothing since Mar 23 on the Win2k smoker http​://www.nntp.perl.org/group/perl.daily-build.reports/ .

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 4, 2014

From @bulk88

On Thu Mar 27 11​:02​:51 2014, bulk88 wrote​:

On Thu Mar 27 06​:01​:15 2014, shay wrote​:

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org>

kill0.t passed for me on no-threads perl with this patch. I
couldn't
make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed now?

Lets wait until the George smoker delivers a new report. I see nothing
since Mar 23 on the Win2k smoker
http​://www.nntp.perl.org/group/perl.daily-build.reports/ .

Mar 23 is still the last smoke. I think it hung. Its been 12 days with no response.
--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @bulk88

On Thu Apr 03 23​:27​:06 2014, bulk88 wrote​:

On Thu Mar 27 11​:02​:51 2014, bulk88 wrote​:

On Thu Mar 27 06​:01​:15 2014, shay wrote​:

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org>

kill0.t passed for me on no-threads perl with this patch. I
couldn't
make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed
now?

Lets wait until the George smoker delivers a new report. I see
nothing
since Mar 23 on the Win2k smoker
http​://www.nntp.perl.org/group/perl.daily-build.reports/ .

Mar 23 is still the last smoke. I think it hung. Its been 12 days
with no response.

George the human responded about problems with the process group kill code in Message-ID​: <alpine.LFD.2.11.1404222133090.31829@​ein.m-l.org> . Its not showing up on nttp.perl.org yet so I can't offer a link.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @Smylers

bulk88 via RT writes​:

George the human responded about problems with the process group kill
code in Message-ID​: <alpine.LFD.2.11.1404222133090.31829@​ein.m-l.org>
. Its not showing up on nttp.perl.org yet so I can't offer a link.

It's http​://nntp.perl.org/group/perl.perl5.porters/214650

For future reference, all P5P emails contain a link to their own web
archive page. View full headers on the above message and you'll see​:

  X-List-Archive​: <http​://nntp.perl.org/group/perl.perl5.porters/214650>

Smylers
--
Why drug companies shouldn't be allowed to hide research results that they
don't like​: http​://gu.com/p/3zat8 — UK gov wasted millions on Tamiflu
Sign the AllTrials petition​: http​://www.alltrials.net/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @demerphq

On 18 March 2014 13​:46, H.Merijn Brand <h.m.brand@​xs4all.nl> wrote​:

On Tue, 18 Mar 2014 13​:15​:11 +0100, demerphq <demerphq@​gmail.com> wrote​:

The difference in parsing time between single and double quotes strings is
infinitesimal

IMO there is no point in worrying about these kind of things unless
the string is very long.

And not even then. I benched it years ago on several machines, and all
bench results are in line-noise margins and differ per architecture.
IIRC all big-endian boxes I tried on had a <1% preference for double
quotes whereas little-endian boxes prefered single quotes.

Well no, when the string becomes long it starts being interesting to
not use a quoted string, or to use single quoted here docs.

When I checked last Perl has non-linear and degrading performance
parsing strings. The longer the string the slower it gets.

In practice unless the string is quite long (hundreds to thousands of
characters) it is not something you need to worry about at all. On the
other hand if you happen to be dealing with a case where you are
generating code with lots of long quoted strings you can see real
parsing performance gains by removing the strings.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @rjbs

* Smylers <Smylers@​stripey.com> [2014-04-23T05​:47​:50]

For future reference, all P5P emails contain a link to their own web
archive page. View full headers on the above message and you'll see​:

X-List-Archive​: <http​://nntp.perl.org/group/perl.perl5.porters/214650>

<jawdrop>

You're my hero of the day!

--
rjbs

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @greerga

On Wed, 23 Apr 2014, bulk88 via RT wrote​:

On Thu Apr 03 23​:27​:06 2014, bulk88 wrote​:

On Thu Mar 27 11​:02​:51 2014, bulk88 wrote​:

On Thu Mar 27 06​:01​:15 2014, shay wrote​:

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org>

kill0.t passed for me on no-threads perl with this patch. I
couldn't
make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed
now?

Lets wait until the George smoker delivers a new report. I see
nothing
since Mar 23 on the Win2k smoker
http​://www.nntp.perl.org/group/perl.daily-build.reports/ .

Mar 23 is still the last smoke. I think it hung. Its been 12 days
with no response.

George the human responded about problems with the process group kill code in Message-ID​: <alpine.LFD.2.11.1404222133090.31829@​ein.m-l.org> . Its not showing up on nttp.perl.org yet so I can't offer a link.

Archives of the logs and reports are always on my web site​:
  http​://m-l.org/~perl/smoke/perl/win32/blead/

Yes, the process group kill code is probably nuking the Win32 blead
smoker. The maint and smoke-me smokers haven't been killed, so the impact
is limited but still annoying. (Kind of like the test in blead that is
hanging and I manually kill every so often.)

As for the reports, I had a hard drive go bad in the RAID5 so I shut the
machine off until the warranty replacement arrived. I updated the status
pages[1] but didn't mail the list as it wasn't "interesting enough".

1​: http​://m-l.org/~perl/smoke/perl/win32/blead/status
  http​://m-l.org/~perl/smoke/perl/linux/blead_g++_quick/status
  ...etc...

--
George Greer

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2014

From @steve-m-hay

On 23 April 2014 14​:17, Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote​:

* Smylers <Smylers@​stripey.com> [2014-04-23T05​:47​:50]

For future reference, all P5P emails contain a link to their own web
archive page. View full headers on the above message and you'll see​:

X-List-Archive​: <http​://nntp.perl.org/group/perl.perl5.porters/214650>

<jawdrop>

You're my hero of the day!

This was news to me too, and would have saved a delay in updating the
epigraphs.pod after the release of 5.19.11 had I known before. I've
therefore updated the RMG with this useful information​:
http​://perl5.git.perl.org/perl.git/commit/70d95cc994e515d77711476f6c853c3f1f1f1458

Thanks!

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 24, 2014

From @iabyn

On Wed, Apr 23, 2014 at 01​:34​:21PM +0200, demerphq wrote​:

On 18 March 2014 13​:46, H.Merijn Brand <h.m.brand@​xs4all.nl> wrote​:

On Tue, 18 Mar 2014 13​:15​:11 +0100, demerphq <demerphq@​gmail.com> wrote​:

The difference in parsing time between single and double quotes strings is
infinitesimal

IMO there is no point in worrying about these kind of things unless
the string is very long.

And not even then. I benched it years ago on several machines, and all
bench results are in line-noise margins and differ per architecture.
IIRC all big-endian boxes I tried on had a <1% preference for double
quotes whereas little-endian boxes prefered single quotes.

Well no, when the string becomes long it starts being interesting to
not use a quoted string, or to use single quoted here docs.

When I checked last Perl has non-linear and degrading performance
parsing strings. The longer the string the slower it gets.

In practice unless the string is quite long (hundreds to thousands of
characters) it is not something you need to worry about at all. On the
other hand if you happen to be dealing with a case where you are
generating code with lots of long quoted strings you can see real
parsing performance gains by removing the strings.

The double-quoted parsing code is very inefficient. It does a lot of
checks for each char position in the string; I suspect we could speed it
up tremendously by doing a strchr() or similar to skip to the next
"interesting" char, and only then doing all the checks.

--
It's not that I'm afraid to die, I just don't want to be there when it
happens.
  -- Woody Allen

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 22, 2016

From @mauke

On Thu Mar 27 11​:02​:51 2014, bulk88 wrote​:

On Thu Mar 27 06​:01​:15 2014, shay wrote​:

On 27 March 2014 07​:41, bulk88 via RT <perlbug-followup@​perl.org>

kill0.t passed for me on no-threads perl with this patch. I
couldn't
make harness finish due to a different bug.

Thanks, applied as 721b267. (A full "nmake test 2>nul" with no
PERL_IMPLICIT_SYS passed all tests for me.)

Does this complete #121230 now? I.e. should the ticket be closed now?

Lets wait until the George smoker delivers a new report. I see nothing
since Mar 23 on the Win2k smoker
http​://www.nntp.perl.org/group/perl.daily-build.reports/ .

What's the status of this ticket? It's listed in perl5200delta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.