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
IPC::Open2, IPC::Open3 feature request: report exec failures separately #10072
Comments
From @epaCreated by @epaWhen using IPC::Open2 or IPC::Open3 to run an external program, use 5.010; It would be good for open2() and open3() to use the same convention as I know that often it is difficult for user code to follow the familiar Perl Info
|
From @epaI should add, I am not sure whether it is possible to implement what I ask for. Getting -1 into the $? variable might be difficult, even though the builtin But I feel there should be some way to report exec() failures correctly with -- ______________________________________________________________________ |
From @ikegamiOn Mon, Jan 11, 2010 at 8:28 AM, Ed Avis <eda@waniasset.com> wrote:
Guess not. |
The RT System itself - Status changed from 'new' to 'open' |
From @ikegamiOn Mon, Jan 11, 2010 at 12:05 PM, Eric Brine <ikegami@adaelis.com> wrote:
Oh, you meant for the child to place -1 in the parent's $?, and the error |
From @epaEric Brine <ikegami@adaelis.com> wrote:
Well, partly that, which is a question of interprocess communication, -- ______________________________________________________________________ |
From @ikegamiOn Mon, Jan 11, 2010 at 12:15 PM, Ed Avis <eda@waniasset.com> wrote:
Maybe if you're trying to set custom messages. If you're trying to set it to $ perl -le'for (1..10) { $!=$_; print $!; }' |
From @ikegamiOn Mon, Jan 11, 2010 at 12:15 PM, Ed Avis <eda@waniasset.com> wrote:
I was wondering how system() managed this. $ strace perl -e'system "nonexistant"' system creates a pipe, over which the value to store in $! is returned. (2, I can write the patch if you want (tomorrow or soon after). - Eric |
From @epaEric Brine <ikegami@adaelis.com> wrote:
I don't completely understand how this works - surely after the child process Yet all sorts of things (including the shell) are able to detect failed exec()
That would be great, I'm very curious to see how it can be done. -- ______________________________________________________________________ |
From ben@morrow.me.ukQuoth eda@waniasset.com (Ed Avis):
The child's end of the pipe is set close-on-exec, so if the pipe is
I don't know how the various shell implementations do it, but I suspect Ben |
From @ikegamiOn Mon, Jan 11, 2010 at 2:53 PM, Eric Brine <ikegami@adaelis.com> wrote:
On Tue, Jan 12, 2010 at 10:01 AM, Ben Morrow <ben@morrow.me.uk> wrote:
Do you know if core provides a means of doing that? |
From ben@morrow.me.ukQuoth ikegami@adaelis.com (Eric Brine):
Either use $^F (which is crude) or set the flag directly with use Fcntl; fcntl $FH, F_SETFD, FD_CLOEXEC; Note that this probably won't work on non-Unix systems. (Are there any systems which define any *other* F_{G,S}ETFD flags, which Ben |
From @ikegamiOn Tue, Jan 12, 2010 at 10:01 AM, Ben Morrow <ben@morrow.me.uk> wrote:
closed with nothing written to it that means the exec succeeded. I had looked for fcntl() in Fcntl, but I hadn't thought to look in perlfunc! Looks like the handle is already close-on-exec in linux, but there's no I can handle Windows, but I know nothing of VMS. Can it fork(), can it Below is what I got t so far. test() returns the same as open3(): An -------------------- BEGIN CODE -------------------- use strict; use Errno qw( EAGAIN ); # Override exec() to simulate failures sub test { pipe(my ($r, $w)) defined( my $pid = fork() ) if (!$pid) { my $flags = fcntl($w, F_GETFD, 0) fcntl($w, F_SETFD, $flags|FD_CLOEXEC) if (!exec(perl => ( -e => 'exit $ARGV[0]', $code ))) { close($w); my $to_read = length(pack('I', 0)); return $pid; my @tests = ( for (@tests) { -------------------- BEGIN OUTPUT -------------------- Test exec failing with $! == 2 Test exec failing with $! == 13 Test exec succeeding and child exiting with code 1 Test exec succeeding and child exiting with code 255 |
From @arcBen Morrow <ben@morrow.me.uk> wrote:
I've investigated that question in the past, and while I can't say -- |
From @ikegamiPatch attached. The first adds TODO tests. |
From @ikegami0001-Add-TODO-test-for-RT-72016.patchFrom 47f948ab7d9571bcf1ad69f23287485ccbaa9674 Mon Sep 17 00:00:00 2001
From: Eric Brine <ikegami@adaelis.com>
Date: Sun, 17 Jan 2010 20:44:14 -0800
Subject: [PATCH 1/2] Add TODO test for RT#72016
---
ext/IPC-Open3/t/IPC-Open3.t | 24 +++++++++++++++++++++++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/ext/IPC-Open3/t/IPC-Open3.t b/ext/IPC-Open3/t/IPC-Open3.t
index 79d5ced..849b0ba 100644
--- a/ext/IPC-Open3/t/IPC-Open3.t
+++ b/ext/IPC-Open3/t/IPC-Open3.t
@@ -47,7 +47,7 @@ my ($pid, $reaped_pid);
STDOUT->autoflush;
STDERR->autoflush;
-print "1..22\n";
+print "1..23\n";
# basic
ok 1, $pid = open3 'WRITE', 'READ', 'ERROR', $perl, '-e', cmd_line(<<'EOF');
@@ -146,3 +146,25 @@ else {
print WRITE "ok 22\n";
waitpid $pid, 0;
}
+
+# RT 72016
+eval{$pid = open3 'WRITE', 'READ', 'ERROR', '/non/existant/program'; };
+if (IPC::Open3::DO_SPAWN) {
+ if ($@ || waitpid($pid, 0) > 0) {
+ print "ok 23\n";
+ } else {
+ print "not ok 23\n";
+ }
+} else {
+ if ($@) {
+ # exec failure should throw exception in parent.
+ print "ok 23 # TODO RT 72016\n";
+ } else {
+ if (waitpid($pid, 0) > 0) {
+ # exec failure currently appears as child error.
+ print "not ok 23 # TODO RT 72016\n";
+ } else {
+ print "not ok 23\n";
+ }
+ }
+}
--
1.6.5.2
|
From @ikegami0002-open3-errors-in-child-croak-parent-RT-72016.patchFrom 03ee1d038782bcdc74ec0ef9d02d895fdc3d34e0 Mon Sep 17 00:00:00 2001
From: Eric Brine <ikegami@adaelis.com>
Date: Mon, 18 Jan 2010 10:21:20 -0800
Subject: [PATCH 2/2] open3 errors in child croak parent RT#72016
Errors in open3 no longer appear to originate from the executed command on forking systems.
---
ext/IPC-Open3/lib/IPC/Open3.pm | 145 ++++++++++++++++++++++++++--------------
ext/IPC-Open3/t/IPC-Open3.t | 11 +--
2 files changed, 99 insertions(+), 57 deletions(-)
diff --git a/ext/IPC-Open3/lib/IPC/Open3.pm b/ext/IPC-Open3/lib/IPC/Open3.pm
index 82c20ae..c367758 100644
--- a/ext/IPC-Open3/lib/IPC/Open3.pm
+++ b/ext/IPC-Open3/lib/IPC/Open3.pm
@@ -48,7 +48,7 @@ instead of a pipe(2) made.
If either reader or writer is the null string, this will be replaced
by an autogenerated filehandle. If so, you must pass a valid lvalue
-in the parameter slot so it can be overwritten in the caller, or
+in the parameter slot so it can be overwritten in the caller, or
an exception will be raised.
The filehandles may also be integers, in which case they are understood
@@ -68,9 +68,9 @@ C<open(FOO, "-|")> the child process will just be the forked Perl
process rather than an external command. This feature isn't yet
supported on Win32 platforms.
-open3() does not wait for and reap the child process after it exits.
+open3() does not wait for and reap the child process after it exits.
Except for short programs where it's acceptable to let the operating system
-take care of this, you need to do this yourself. This is normally as
+take care of this, you need to do this yourself. This is normally as
simple as calling C<waitpid $pid, 0> when you're done with the process.
Failing to do this can result in an accumulation of defunct or "zombie"
processes. See L<perlfunc/waitpid> for more information.
@@ -161,6 +161,18 @@ sub xpipe {
pipe $_[0], $_[1] or croak "$Me: pipe($_[0], $_[1]) failed: $!";
}
+sub xpipe_anon {
+ pipe $_[0], $_[1] or croak "$Me: pipe failed: $!";
+}
+
+sub xclose_on_exec {
+ require Fcntl;
+ my $flags = fcntl($_[0], &Fcntl::F_GETFD, 0)
+ or croak "$Me: fcntl failed: $!";
+ fcntl($_[0], &Fcntl::F_SETFD, $flags|&Fcntl::FD_CLOEXEC)
+ or croak "$Me: fcntl failed: $!";
+}
+
# I tried using a * prototype character for the filehandle but it still
# disallows a bearword while compiling under strict subs.
@@ -199,12 +211,12 @@ sub _open3 {
unless (eval {
$dad_wtr = $_[1] = gensym unless defined $dad_wtr && length $dad_wtr;
$dad_rdr = $_[2] = gensym unless defined $dad_rdr && length $dad_rdr;
- 1; })
+ 1; })
{
# must strip crud for croak to add back, or looks ugly
$@ =~ s/(?<=value attempted) at .*//s;
croak "$Me: $@";
- }
+ }
$dad_err ||= $dad_rdr;
@@ -225,54 +237,89 @@ sub _open3 {
xpipe $dad_rdr, $kid_wtr if !$dup_rdr;
xpipe $dad_err, $kid_err if !$dup_err && $dad_err ne $dad_rdr;
- $kidpid = DO_SPAWN ? -1 : xfork;
- if ($kidpid == 0) { # Kid
- # A tie in the parent should not be allowed to cause problems.
- untie *STDIN;
- untie *STDOUT;
- # If she wants to dup the kid's stderr onto her stdout I need to
- # save a copy of her stdout before I put something else there.
- if ($dad_rdr ne $dad_err && $dup_err
- && xfileno($dad_err) == fileno(STDOUT)) {
- my $tmp = gensym;
- xopen($tmp, ">&$dad_err");
- $dad_err = $tmp;
- }
+ if (!DO_SPAWN) {
+ # Used to communicate exec failures.
+ xpipe my $stat_r, my $stat_w;
+
+ $kidpid = xfork;
+ if ($kidpid == 0) { # Kid
+ eval {
+ # A tie in the parent should not be allowed to cause problems.
+ untie *STDIN;
+ untie *STDOUT;
+
+ close $stat_r;
+ xclose_on_exec $stat_w;
+
+ # If she wants to dup the kid's stderr onto her stdout I need to
+ # save a copy of her stdout before I put something else there.
+ if ($dad_rdr ne $dad_err && $dup_err
+ && xfileno($dad_err) == fileno(STDOUT)) {
+ my $tmp = gensym;
+ xopen($tmp, ">&$dad_err");
+ $dad_err = $tmp;
+ }
+
+ if ($dup_wtr) {
+ xopen \*STDIN, "<&$dad_wtr" if fileno(STDIN) != xfileno($dad_wtr);
+ } else {
+ xclose $dad_wtr;
+ xopen \*STDIN, "<&=" . fileno $kid_rdr;
+ }
+ if ($dup_rdr) {
+ xopen \*STDOUT, ">&$dad_rdr" if fileno(STDOUT) != xfileno($dad_rdr);
+ } else {
+ xclose $dad_rdr;
+ xopen \*STDOUT, ">&=" . fileno $kid_wtr;
+ }
+ if ($dad_rdr ne $dad_err) {
+ if ($dup_err) {
+ # I have to use a fileno here because in this one case
+ # I'm doing a dup but the filehandle might be a reference
+ # (from the special case above).
+ xopen \*STDERR, ">&" . xfileno($dad_err)
+ if fileno(STDERR) != xfileno($dad_err);
+ } else {
+ xclose $dad_err;
+ xopen \*STDERR, ">&=" . fileno $kid_err;
+ }
+ } else {
+ xopen \*STDERR, ">&STDOUT" if fileno(STDERR) != fileno(STDOUT);
+ }
+ return 0 if ($cmd[0] eq '-');
+ exec @cmd or do {
+ local($")=(" ");
+ croak "$Me: exec of @cmd failed";
+ };
+ };
+
+ my $bang = 0+$!;
+ my $err = $@;
+ utf8::encode $err if $] >= 5.008;
+ print $stat_w pack('IIa*', $bang, length($err), $err);
+ close $stat_w;
- if ($dup_wtr) {
- xopen \*STDIN, "<&$dad_wtr" if fileno(STDIN) != xfileno($dad_wtr);
- } else {
- xclose $dad_wtr;
- xopen \*STDIN, "<&=" . fileno $kid_rdr;
- }
- if ($dup_rdr) {
- xopen \*STDOUT, ">&$dad_rdr" if fileno(STDOUT) != xfileno($dad_rdr);
- } else {
- xclose $dad_rdr;
- xopen \*STDOUT, ">&=" . fileno $kid_wtr;
+ eval { require POSIX; POSIX::_exit(255); };
+ exit 255;
}
- if ($dad_rdr ne $dad_err) {
- if ($dup_err) {
- # I have to use a fileno here because in this one case
- # I'm doing a dup but the filehandle might be a reference
- # (from the special case above).
- xopen \*STDERR, ">&" . xfileno($dad_err)
- if fileno(STDERR) != xfileno($dad_err);
- } else {
- xclose $dad_err;
- xopen \*STDERR, ">&=" . fileno $kid_err;
+ else { # Parent
+ close $stat_w;
+ my $to_read = length(pack('I', 0)) * 2;
+ my $bytes_read = read($stat_r, my $buf = '', $to_read);
+ if ($bytes_read) {
+ (my $bang, $to_read) = unpack('II', $buf);
+ read($stat_r, my $err = '', $to_read);
+ if ($err) {
+ utf8::decode $err if $] >= 5.008;
+ } else {
+ $err = "$Me: " . ($! = $bang);
+ }
+ $! = $bang;
+ die($err);
}
- } else {
- xopen \*STDERR, ">&STDOUT" if fileno(STDERR) != fileno(STDOUT);
}
- return 0 if ($cmd[0] eq '-');
- local($")=(" ");
- exec @cmd or do {
- carp "$Me: exec of @cmd failed";
- eval { require POSIX; POSIX::_exit(255); };
- exit 255;
- };
- } elsif (DO_SPAWN) {
+ }
+ else { # DO_SPAWN
# All the bookkeeping of coincidence between handles is
# handled in spawn_with_handles.
diff --git a/ext/IPC-Open3/t/IPC-Open3.t b/ext/IPC-Open3/t/IPC-Open3.t
index 849b0ba..23ca8e5 100644
--- a/ext/IPC-Open3/t/IPC-Open3.t
+++ b/ext/IPC-Open3/t/IPC-Open3.t
@@ -157,14 +157,9 @@ if (IPC::Open3::DO_SPAWN) {
}
} else {
if ($@) {
- # exec failure should throw exception in parent.
- print "ok 23 # TODO RT 72016\n";
+ print "ok 23\n";
} else {
- if (waitpid($pid, 0) > 0) {
- # exec failure currently appears as child error.
- print "not ok 23 # TODO RT 72016\n";
- } else {
- print "not ok 23\n";
- }
+ waitpid($pid, 0);
+ print "not ok 23\n";
}
}
--
1.6.5.2
|
From @iabynEric notes that he has pending work on this ticket |
From @ikegamiOn Tue Mar 16 03:07:52 2010, davem wrote:
More precisely, the patches are written, attached to the ticket and |
From @rgarciaOn 18 January 2010 20:38, Eric Brine <ikegami@adaelis.com> wrote:
Thanks, applied to blaedperl. I also bumped the version of IPC::Open3 to 1.06. |
@rgs - Status changed from 'open' to 'resolved' |
From @epa
This doesn't seem to have made it into perl-5.12.3, which still includes Could I ask you to look at why this didn't make it into perl proper? |
From @ikegamiOn Tue, Feb 15, 2011 at 11:43 AM, Ed Avis via RT
There has only been maintenance releases since this was changed. Only |
From @epa
Ah, I see. The patch went in shortly after 5.12 had been released, so Is it straightforward to release the new version on CPAN in the |
From @ikegamiOn Wed, Feb 16, 2011 at 11:06 AM, Ed Avis via RT
Correct.
It's not a dual-life module, i.e. it's only distributed as part of Perl If you want it now, just grab the .pm from git and save it into your lib - Eric |
From @epaThanks to all who worked on this. What happens now is that open3() throws an exception. open3: exec of nonexistent failed: No such file or directory |
Migrated from rt.perl.org#72016 (status was 'resolved')
Searchable as RT72016$
The text was updated successfully, but these errors were encountered: