Segfault if watcher is GCed during execution with uncaught exception

[kelunik]

The following test fails and breaks a test in Amp based code:

--TEST--
Check for bug #44
--FILE--
<?php

$ev = new \EvLoop();

$exception = new \Exception('test');

$t0 = $ev->timer(0.1, 0.1, function () use ($exception, &$t0) {
    $t0->stop();
    $t0 = null;

    throw $exception;
});

try {
    $ev->run(\Ev::RUN_ONCE);
} catch (\Exception $e) {
    self::assertSame($exception, $e);
}

--EXPECT--

‌

[Ruslan Osmanov]

This happens because pecl-ev tries to stop the watcher in case if the callback throws an exception. Otherwise, the loop would block in an infinite loop if the user doesn’t catch the exception (see #43).

Although you shouldn’t destroy the watcher in its callback ($t0 = null;), I have to admit that the the bug report is still relevant – segfaults should never happen.

So now we should somehow detect if the watcher is destroyed before trying to stop it:

        if (EG(exception)) {
            php_error_docref(NULL, E_WARNING,
                    "Stopping %s watcher because of uncaught exception in the callback",
                    ZSTR_VAL(Z_OBJCE_P(&php_ev_watcher_self(watcher))->name));
            // XXX if (detect_if_watcher_is_destroyed_despite_of_the_garbage_after_freeing(watcher))
            php_ev_stop_watcher(watcher);
        }

I’d be glad if somebody could come up with a solution to this.

For the time being, I’m leaving this ticket open until I have some more free time.

[Ruslan Osmanov]

@kelunik , I’m trying to remember why I decided to stop only the watcher instead of the whole event loop

Exceptions are actually thrown on the call to Ev::run. If some of the watchers throws an exception, the flow leaves the line where the Ev::run() was called. So we probably should stop the loop and all of its inner loops using ev_break(loop, EVBREAK_ALL), what do you think?

[kelunik]

Letting them bubble up to Loop::run() / Ev::run() is what we have in Amp as well for every loop implementation. IMO there's no need to stop the watcher. If the user calls Ev::run again without stopping the watcher and it throws again, it's reasonable to get a exception again.

Having this behavior consistent between all implementations would be highly appreciated.

[Ruslan Osmanov]

Fixed bug #44

→ <<cset 4fc69c114b42>>

[Ruslan Osmanov]

@kelunik, if I understand you correctly, you are okay with the fact that the event loop will be stopped when an exception is thrown from the watcher callback.

At least, we must to stop the loop. Otherwise, the flow will not be able to run out of the blocking Ev::run() or EvLoop::run(). I’ve pushed the fix to master. It would be great if you could check it before I publish a new PECL release. You can use the docker and docker compose configurations as follows:

cat test.php | docker-compose run php7test ./run.sh

Please share your thoughts. If it looks good, I’ll create and publish a new PECL package.

[kelunik]

Yes, that's fine behavior IMO. I can compile from source, don't worry. I'll give it a test later this week.