From 55fe1825bcbfef1b5931af2b5c30c2ab299f4bea Mon Sep 17 00:00:00 2001
From: Diab Jerius <djerius@cfa.harvard.edu>
Date: Mon, 17 Feb 2025 23:23:47 -0500
Subject: [PATCH] Ensure that all FH are closed and the subprocess is reaped
 upon object destruction.

IO objects are special, and sometimes are not destroyed when
references to them disappear.

Here's some example code:

{
  my $cmd = System::Command->new( 'cat' );
  $cmd->stdin->say( $_ ) for 0 ... 100;
};

When the block ends, $cmd will be destroyed.  Sometimes the
order of destruction is

- System::Command
- IO::Handle
- System::Command::Reaper

and sometimes it is

- System::Command
- System::Command::Reaper

Experimentally, when the Reaper object is destroyed (regardless of
whether the IO::Handles are destroyed first) it has lost access to its
cached IO::Handle objects.  They are cached as weak references, and
when the Command object is destroyed, it releases its references to
them, and the Reaper object loses its refereces.

If the second order of destruction happens, the handles still exist
(Perl seems to have an internal cache for them), but Reaper doesn't
see them, and thus can't close them.  It will try to reap the child
process, but if the child is awaiting input, it will never exit,
cannot be reaped, and Reaper::_reap will hang forever.

(Note that explicitly closing the handles via $cmd->close
is not safe, as, if the program is interrupted before the close,
and the signal handler throws, e.g. $SIG{INT} = sub { die },
the $cmd->close does not get fired.

So, just make sure things are closed.
---
 lib/System/Command.pm | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/System/Command.pm b/lib/System/Command.pm
index 7b6a8a8..e22ed14 100644
--- a/lib/System/Command.pm
+++ b/lib/System/Command.pm
@@ -376,6 +376,49 @@ sub close {
     return $_[0];
 }
 
+sub DESTROY {
+
+    # ensure that all FH are closed. IO objects are special, and
+    # sometimes are not destroyed when references to them disappear.
+
+    # Here's some example code:
+
+    # {
+    #   my $cmd = System::Command->new( 'cat' );
+    #   $cmd->stdin->say( $_ ) for 0 ... 100;
+    # };
+
+    # When the block ends, $cmd will be destroyed.  Sometimes the
+    # order of destruction is
+
+    # System::Command, IO::Handle, System::Command::Reaper
+
+    # and sometimes it is
+
+    # System::Command, System::Command::Reaper
+
+    # experimentally, when the Reaper object is destroyed ( regardless
+    # of whether the IO::Handles are destroyed first) it never has
+    # access to its cached IO::Handle objects; when the Command object
+    # is destroyed, it releases its references to them, and because
+    # the Reaper object only has weak references to them, they
+    # disappear.
+
+    # So, if the IO::Handle destructor isn't called, the handles still
+    # exist, but Reaper doesn't close them, and if the subprocess is
+    # waiting on input, it will never exit, cannot be reaped,
+    # and Reaper::_reap will hang forever.
+
+    # note that explicitly closing the handles via $cmd->close
+    # is not safe, as, if the program is interrupted before the close,
+    # and the signal handler throws, e.g. $SIG{INT} = sub { die },
+    # the $cmd->close does not get fired.
+
+    # so, just make sure things are closed
+
+    $_[0]{reaper} and $_[0]->close;
+}
+
 1;
 
 __END__