Beefy Boxes and Bandwidth Generously Provided by pair Networks
We don't bite newbies here... much
 
PerlMonks  

Re^3: Open3 and bad gut feeling : Finally

by ikegami (Pope)
on Sep 15, 2004 at 00:30 UTC ( #391034=note: print w/ replies, xml ) Need Help??


in reply to Re: Re: Open3 and bad gut feeling : Finally
in thread Open3 and bad gut feeling

This code can be trimmed down a lot.

  • $stdin, $stdout and $stderr are never used, so they should be deleted.
  • Flushing does nothing for input handles, so $dout->autoflush(1);, $derr->autoflush(1);, $dout->flush(); and $derr->flush(); do nothing.
  • $din->flush(); is useless for two reason: 1) The program on the other side of the pipe has ended, and 2) the close does a flush.
  • $din->autoflush(1); is not needed since we don't write to $din. Even if we did, open3 makes it autoflush for us.
  • Since we don't do anything with $din, $dout and $derr anymore, we can let open3 create them for us.
  • Filehandles are automatically closed when they go out of scope.
  • The $pid is for a dead process, so why return it to the caller?
  • It's more common to use 0/1 for boolean instead of "false"/"true". Deviations make it less readable, and strings make it more complicated. Using a regular expression to check the equality of two strings is overkill. eq is the code for that.
  • Reformatting $out and $err has nothing to do with OSexecute and should be done by the calling function if it so desires.
  • There are many small problems.

  • $stdin->add("\*STDIN"); should be $stdin->add(\*STDIN);.
  • $val = waitpid(-1, 0); should be $val = waitpid($pid, 0);. $val isn't used, so the assignment can be ommited as well.
  • "@execute" will cram all the arguments into one word.
  • Why change a stdout and stderr to 1 if it's empty, when it's just as easy to check for the empty string ($str eq '' and length($str)
  • $stdout is changed from to 1 if it's '0'. Same for $stderr.
  • Returning a hash is... odd. If the code did return ($stdout, $stderr);, the caller could very simply do my ($stdout, $stderr) = OSexecute(...);
  • Cleaned Up Code

    # RETURNS: ( $stdout, $stderr ) Both are refs to an array of lines. sub OSexecute { my @execute = @_; local $, = ' '; # for print "...@execute..." my $debug = 0; #$debug = 1; print("OSexecute(@execute)\n") if ($debug); # Here is the actual execution. my $pid = eval { open3($din, $dout, $derr, @execute) }; die "OSexecute(@execute): $@" if ($@); # Wait for process to complete. waitpid($pid, 0); # Gather the results my @stdout = <$dout>; my @stderr = <$derr>; # We should check the return code of the child. # Gotta trap SIGPIPE for that. return ( \@stdout, \@stderr ); }

    There's a big problem

    { my $file_name; foreach $file_name ('c:\\tinyfile.txt', 'c:\\biggfile.txt') { my ($stdout, $stderr); print("$file_name\n", ("=" x length($file_name))."\n", "\n"); ($stdout, $stderr) = OSexecute('cmd.exe', '/c', 'type', "\"$file_name\""); print("stdout\n", "------\n", @$stdout); print("Nothing was sent to STDOUT.\n") unless (@$stdout); print("\n"); print("stderr\n", "------\n", @$stderr); print("Nothing was sent to STDERR.\n") unless (@$stderr); print("\n", "\n"); } }

    outputs

    c:\tinyfile.txt =============== stdout ------ foo bar bla stderr ------ Nothing was sent to STDERR. c:\biggfile.txt =============== *** HANGS ***

    The problem is that file handles (including $dout and $derr) have a buffer that's limited in size. biggfile.txt is less than 2KB, which is really quite small, so this needs to be fixed.

    Fix

    # RETURNS: ( $stdout, $stderr ) Both are refs to an array of lines. sub OSexecute { my @execute = @_; my @stdout; my @stderr; local $, = ' '; # for print "...@execute..." my $debug = 0; #$debug = 1; print("OSexecute(@execute)\n") if ($debug); # Here is the actual execution. my $pid = eval { open3($din, $dout, $derr, @execute) }; die "OSexecute(@execute): $@" if ($@); my $select = IO::Select->new(); $select->add($dout); $select->add($derr); my @ready; my $fh; # Gather the results while (@ready = $select->can_read()) { foreach $fh (@ready) { push(@stdout, <$fh>) if ($fh == $dout); push(@stderr, <$fh>) if ($fh == $derr); } } # Wait for process to complete and reap it. waitpid($pid, 0); # We should check the return code of the child. # Gotta trap SIGPIPE for that. return ( \@stdout, \@stderr ); }

    Fixed?

    Oops! select() (and IO::Select) doesn't work in Windows. That sucks. It means we're gonna have to use threads!!! I have no experience with threads, so maybe another day.


    Comment on Re^3: Open3 and bad gut feeling : Finally
    Select or Download Code
    Re^4: Open3 and bad gut feeling : Finally
    by coreolyn (Parson) on Sep 15, 2004 at 12:01 UTC

      Big Sincere Thank You for your comments and thoughts on this node!

      As I mentioned earlier I had no problems with it and have exipirienced no issues other than the 'hang' on NT. It has come up a couple of times in the last couple of years but rerunning the base app has worked on both occasions, and I've always had $stderr returned.

      (As for returning the hash... I created a logging scheme ( Prior to log4 pardigms that worked with the hash as a parameter)

      I will make some time to dig back into this now that I've finally gotten somethings to ponder and consider.

    Log In?
    Username:
    Password:

    What's my password?
    Create A New User
    Node Status?
    node history
    Node Type: note [id://391034]
    help
    Chatterbox?
    and the web crawler heard nothing...

    How do I use this? | Other CB clients
    Other Users?
    Others romping around the Monastery: (14)
    As of 2015-07-06 18:26 GMT
    Sections?
    Information?
    Find Nodes?
    Leftovers?
      Voting Booth?

      The top three priorities of my open tasks are (in descending order of likelihood to be worked on) ...









      Results (80 votes), past polls