Hi Ken,

Just to pick up on what the AM mentioned: properly closeing a piped open is important so one can catch all possible errors (as opposed to closeing regular files, where in my experience things go wrong much less often). I see you used autodie, but as far as I can tell it doesn't catch errors when the filehandle is implicitly closed when it goes out of scope.

In your code, if I replace the command with something that returns a nonzero exit code, and add the close at the end of the sub, I get confusing results: inside of capture, the error doesn't seem to get caught at all, and outside of capture, I get the confusing error message "Can't close(GLOB(0x8e2b62)) filehandle: '' at ...". (In fact, if I remember correctly, strange interactions with autodie and piped opens is one of the reasons I started avoiding autodie.)

So here's the same run_external_command code, without autodie but with the minimum error handling:

my $extcmd = 'cat';
open my $cmd_pipe, '|-', $extcmd or die "open $extcmd: $!";
print $cmd_pipe "password\n";
close $cmd_pipe or die "close $extcmd: ".($! ? $! : "\$?=$?");
[download]

However, nowadays I very much prefer to use more better-suited modules, one of my favorites is IPC::Run3. For the OP:

use IPC::Run3 'run3';
my $stdin = "password\n";
my @extcmd = ('cat');
run3 \@extcmd, \$stdin, \my $stdout, \my $stderr
    or die "run3 failed";
$? and die "run3: \$?=$?";
print "stdout: <<$stdout>>\n";
print "stderr: <<$stderr>>\n";
[download]

Regards,
-- Hauke D

G'day Hauke,

Thanks for the feedback.

In my original test code (unpublished), I did have

close $cmd_pipe;
[download]

and I started to add some error checking. However, realising that I had no idea what the OP's external command was nor what error codes he might be looking for, I ripped that all out and just posted the part answering the original question. I probably should have left the close statement.

I don't know what autodie is doing under the hood; however, given that

open my $cmd_pipe, '|-', $external_command;
[download]

performs a fork, I suspect it's checking that (in its :threads category) and not doing anything in the :system category.

The only mention of pipe that I can see in autodie is that it's in the :ipc category. There's also one instance in IPC::System::Simple that I found somewhat humourous:

"Implementing the capture command involves dark and terrible magicks involving pipes, and one of them has sprung a leak."

[That's &IPC::System::Simple::capture, not &Capture::Tiny::capture.]

"perlipc: Using open() for IPC" has more information on error checking:

"Be careful to check the return values from both open() and close(). If you're writing to a pipe, you should also trap SIGPIPE. ..."

See also: "perlvar: Error Variables".

"However, nowadays I very much prefer to use more better-suited modules, ..."

Yes, Capture::Tiny wouldn't have been my first choice for this task; however, the OP mentions it four times in his single-paragraph post, so that's what I used.

— Ken

Hi Ken,

Usually I tend to code defensively, especially when it comes to running external commands, I like to know every single time something goes wrong - I'm one of those people who usually looks at all the mails I get from e.g. cron :-)

Having had problems with autodie in the past, I tend to avoid it nowadays, and write the error handling myself. Unfortunately, at the moment I don't recall the specific problems (other than this particular case) I had with it, just that it was "spooky action-at-a-distance" type stuff.

On the other hand, I've also had problems with piped opens, such as it not working when given a Readonly variable (which I reported back then). The other reason I don't use piped opens anymore is that I've found that I'm usually not just interested in the command's STDOUT, but also its STDERR. (One of my modules, IPC::Run3::Shell, even includes a fail_on_stderr option, which considers any output to STDERR an error condition, something I've used frequently.)

So having been burned a little by autodie, Readonly, and piped opens, I instead, respectively, write the error handling myself, give the user just enough rope to shoot themselves in the foot, and use modules like IPC::System::Simple, Capture::Tiny, and IPC::Run3 instead :-)

Regards,
-- Hauke D

"This works, but you can't trust the exit value returned as the 3rd output value of capture and must instead inspect $?."

Well, the documentation for Capture::Tiny v0.44 (the latest version, at the time of writing, and the version I used) states:

"The capture function takes a code reference and returns what is sent to STDOUT and STDERR as well as any return values from the code reference." [my emphasis]

I took this to be correct but maybe you have other information. Please share.

I did a quick test. The third value, when captured, did contain the return value of the code reference. Could you provide some code showing this third value with something other than the return value of the code reference.

— Ken

Hi,

but you can't trust the exit value returned as the 3rd output value of capture and must instead inspect $?

I'm guessing what you're comparing kcott's code to is this example from the Capture::Tiny doc:

($stdout, $stderr, $exit) = capture {
  system( $cmd, @args );
};
[download]

In that code, the reason that you get $? stored into $exit is that system returns the same thing as $?, and capture returns the return value(s) from the block of code. Since in kcott's code the last statement in the block is print, its return value is what capture returns as the 3rd return value. If you wanted to use the above pattern of getting $? as the 3rd return value, just change kcott's code like this: sub run_external_command { ...; $? }.

Hope this helps,
-- Hauke D

FYI, Capture::Tiny::Extended resolves the collection of the return code.
From the doc:

Capture::Tiny::Extended automatically captures return values and returns them after the second return value (or first if you're using the merged functions).

use Capture::Tiny::Extended 'capture';
 
my ( $out, $err, $res ) = capture { system( 'ls' ) };
[download]

        ...it is unhealthy to remain near things that are in the process of blowing up.     man page for WARP, by Larry Wall

Capture::Tiny can already do this; this feature was added in 2011 in a patch from Christian Walde, the author of Capture::Tiny::Extended (also released in 2011).