Unexpected finish emitted when using subprocess

[HEM42]

• Mojolicious version: 8.58
• Perl version: 5.32.0
• Operating system: MacOS v10.15.6

Steps to reproduce the behavior

I have a route which triggers some longer running IOLoop subprocesses, for which I return promises. I have a subsystem I need to log into at the begining, somewhere along the code I can abort due to other errors, and I always need to logout of this subsystem when the transaction with the user is over. For this I use $c->on( finish => sub{} );

The problem is that ->on( finish is triggered for each subprocess running, and I dont want to logout of the subsystem before everything is done / or some error occurred.

#!/usr/bin/env perl

use Mojo::Base -strict;
use Test::Mojo;
use Test::More;

use Mojo::Promise;
use Mojolicious::Lite -signatures;

get '/' => sub ($c) {
    $c->render_later;
    $c->on(
        finish => sub {
            say 'FINISHED';
            # $c->subsystem->logout;
        }
    );

    # $c->subsystem->login; to some subsystem

    my $p1 = another_long_running_subprocess();
    my $p2 = another_long_running_subprocess();
    my $p3 = another_long_running_subprocess();

    Mojo::Promise->all( $p1, $p2, $p3 )->then(
        sub (@results) {
            # manage results
            $c->render( text => 'Hello World!' );
        }
    )->wait;
};

my $t = Test::Mojo->new;

$t->get_ok('/')->status_is(200);

done_testing;

sub another_long_running_subprocess
{
    my $subprocess = Mojo::IOLoop::Subprocess->new;

    my $promise = $subprocess->run_p(
        sub {
            my $result = `ping -c 5 1.1.1.1`;
            return $result;
        },
    );

    return $promise;
}

Expected behavior

I would not expect to see more one finish emitted

Actual behavior

Instead of a single finish, one from each subprocess is also emitted

Output from the above code example

FINISHED
FINISHED
FINISHED
FINISHED
ok 1 - GET /
ok 2 - 200 OK
1..2

[kraih]

Why are you not using Mojo::IOLoop->subprocess?

[HEM42]

Why are you not using Mojo::IOLoop->subprocess?

No specific reason, but changing the code still produce the same result

sub another_long_running_subprocess
{
    Mojo::IOLoop->subprocess->run_p(
        sub {
            my $result = `ping -c 5 1.1.1.1`;
            return $result;
        },
    );
}

[kiwiroy]

@HEM42 A single "FINISHED" with diff.

[HEM42]

@HEM42 A single "FINISHED" with diff.

Nice as a workaround.

[kraih]

Not really a solution, but it does seem likely that we will have to limit some cleanup code by $$.

[kiwiroy]

Needs a weaken to pass tests if it's put in here.

diff --git a/lib/Mojolicious/Controller.pm b/lib/Mojolicious/Controller.pm
index 3395d88ba..dcdf444bb 100644
--- a/lib/Mojolicious/Controller.pm
+++ b/lib/Mojolicious/Controller.pm
@@ -114,9 +114,11 @@ sub finish {
 sub helpers { $_[0]->app->renderer->get_helper('')->($_[0]) }

 sub on {
-  my ($self, $name, $cb) = @_;
+  my ($pid, $self, $name, $cb) = ($$, @_);
   my $tx = $self->tx || Carp::croak 'Transaction already destroyed';
   $self->rendered(101) if $tx->is_websocket && !$tx->established;
+  Scalar::Util::weaken $tx;
+  Mojo::IOLoop->singleton->on(reset => sub { return if $$ == $pid; $tx->unsubscribe($name); });
   return $tx->on($name => sub { shift; $self->$cb(@_) });
 }

[kraih]

That's still just a bandaid. A proper solution will be something low level in Mojo::IOLoop::Stream or so, simplifying the cleanup process if $$ changed.

[kraih]

Or Mojo::IOLoop->reset could be changed to not trigger events on cleanup.

[kiwiroy]

Mojo::IOLoop::Stream 8d4cfe7 or Mojo::IOLoop 9acf79e

[kraih]

@kiwiroy The first proposal makes sense. But i really don't get the second, why doesn't that just run the cleanup code from where the reset event is emitted in the first place?

[kiwiroy]

@kraih correct that cleanup code can be far more brief in Mojo::IOLoop 0af26bf, no closing over a copy of $$ is required. This in mind, I'll open a PR for consideration.

[HEM42]

Ping, any progress on a final solution for this issue ?

[kraih]

The merged fix turned out to be a huge mistake that caused many regressions and had to be reverted. So far nobody has made an alternative proposal.

[kraih]

I made a proof of concept for a better solution. Instead of destroying everything it just freezes the current state of the event loop after fork to prevent side effects. https://github.com/mojolicious/mojo/compare/freeze_ioloop

It currently only works for the singleton instance though, and i don't have time to figure out how to make it work for arbitrary instances. If anyone wants to take over and make it work you're welcome to.

[kraih]

You'd probably want to freeze the internal state of the current instance, then clear it and only replace its reactor. 🤔

[kraih]

Actually, just writing about a possible approachmight have made me figure it out. 🤣 https://github.com/mojolicious/mojo/compare/freeze_ioloop_instance

[kraih]

Probably resolved now.