Leaks and eventually runs out of file descriptors due to a ref cycle

[maaarghk]

Hi,

the docs don't mention this so I don't think I am "holding it wrong". If I do this:

sub do_a_thing
{
    my $self = shift;
    my $process = process(execute => '/usr/local/bin/whatever')->args(['arg', 'arg']);
    $self->child_process($process);
    $process->on(stop => sub {
        my $process = shift;
        if ($process->exit_status)
        {
            warn "child failed: " . $process->exit_status;
            $self->clear_child_process;
            return;
        }
        my $output = $process->read_stdout;
        warn $output;
        $self->clear_child_process;
        return;
    });
    $process->start;
}

After the process exits, I don't hold any handles to $process, so I'd expect it to go out of scope and eventually lose the file descriptors for the IPC pipes. But it doesn't, because of a cycle:

Cycle (1):
        $Mojo::IOLoop::ReadWriteProcess::A->{'session'} => \%Mojo::IOLoop::ReadWriteProcess::Session::B
        $Mojo::IOLoop::ReadWriteProcess::Session::B->{'process_table'} => \%C
                          $C->{'2160'} => \$D
                                   $$D => \%Mojo::IOLoop::ReadWriteProcess::A

and this never gets called:

package Mojo::IOLoop::ReadWriteProcess;

sub DESTROY
{
    warn "destroying $_[0]";
}

I can resolve this manually by calling $process->session->clean, but I think the "proper" fix is for this ref to be weakened immediately after it is created:

Mojo-IOLoop-ReadWriteProcess/lib/Mojo/IOLoop/ReadWriteProcess/Session.pm

Lines 101 to 105 in 5831e64

	sub register {
	my ($process, $pid) = (pop, pop);
	$singleton->process_table()->{$pid} = \$process;
	$singleton->emit(register => $process);
	}

I've verified the following monkey patch works which means "weaken($process)" above would be a bare minimum fix:

package Mojo::IOLoop::ReadWriteProcess::Session;

use Scalar::Util qw(weaken);

sub register
{
    my $self = shift;
    my $pid = shift;
    my $process = shift;
    $self->process_table()->{$pid} = \$process;
    weaken($process);
    $self->emit(register => $process);
}

But there's probably a wider architectural question, since process "registers" itself to the session when it starts, perhaps it should also "unregister" itself from the session when it stops so it's not just relying on going out of scope to drop the (by then defunct) pipes.