- edited description
Segfault if watcher is GCed during execution with uncaught exception
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--
Comments (7)
-
reporter -
repo owner 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.
-
repo owner @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 theEv::run()
was called. So we probably should stop the loop and all of its inner loops usingev_break(loop, EVBREAK_ALL)
, what do you think? -
reporter 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.
-
repo owner - changed status to resolved
Fixed bug
#44→ <<cset 4fc69c114b42>>
-
repo owner @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()
orEvLoop::run()
. I’ve pushed the fix tomaster
. 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.
-
reporter Yes, that's fine behavior IMO. I can compile from source, don't worry. I'll give it a test later this week.
- Log in to comment