reactphp-child-process-messenger icon indicating copy to clipboard operation
reactphp-child-process-messenger copied to clipboard

Exceptions thrown in calls to Messenger::callRpc() from Rpc::handle() are not handled

Open MatteoOreficeIT opened this issue 6 years ago • 3 comments

Hi

I discovered that the call to callRpc in \WyriHaximus\React\ChildProcess\Messenger\Messages\Rpc::handle doesn't handle exceptions thrown in user code ( closures registerd with registerRpc )

That's because Promise:done() of a FulfilledPromise should not never throw an exception ( the $onFulfilled callable in done() method of a FulfilledPromise is immediately called )

$this->callRpc($target, $payload)->done(
    function (array $payload) use ($uniqid,$target) {
        $this->write($this->createLine(Factory::rpcSuccess($uniqid, $payload)));
    },
    function ($error) use ($uniqid) {
        $this->write($this->createLine(Factory::rpcError($uniqid, $error)));
    },
    function ($payload) use ($uniqid) {
        $this->write($this->createLine(Factory::rpcNotify($uniqid, $payload)));
    }
);

A possible fix would be

try {
    $this->callRpc($target, $payload)->then(
        function (array $payload) use ($uniqid,$target) {
            $this->write($this->createLine(Factory::rpcSuccess($uniqid, $payload)));
        },
        function ($error) use ($uniqid) {
            $this->write($this->createLine(Factory::rpcError($uniqid, $error)));
        },
        function ($payload) use ($uniqid) {
            $this->write($this->createLine(Factory::rpcNotify($uniqid, $payload)));
        }
    );
}
catch (\Throwable $throwable) {
    $this->write($this->createLine(Factory::rpcError($uniqid, $throwable)));
}

To reproduce a similar error for example code in the ChildInterface an rpc with a closure throwing some \Error , for example

Argument 1 passed to WyriHaximus\React\ChildProcess\Messenger\Messages\Rpc::WyriHaximus\React\ChildProcess\Messenger\Messages{closure}() must be of the type array, string given, called in /usr/src/myapp/vendor/react/promise/src/FulfilledPromise.php on line 39

$messenger->registerRpc('example', function (Payload $payload) {
    return \React\Promise\resolve('i am not an array');
});

these kind of errors will be totally skipped and parent process will not notify any problem

MatteoOreficeIT avatar May 09 '19 12:05 MatteoOreficeIT

Hey @MatteoOreficeIT sorry for the late reply, been at PHPDay in Italy the past weekend and still recovering. Looks like you found a serious bug! Could you write up a PR with tests ensuring this doesn't happen again?

About the possible fix, it might be simpler to do ensuring we catch the errors in the then as well:

$this->callRpc($target, $payload)->done(
    function (array $payload) use ($uniqid,$target) {
        $this->write($this->createLine(Factory::rpcSuccess($uniqid, $payload)));
    },
    null,
    function ($payload) use ($uniqid) {
        $this->write($this->createLine(Factory::rpcNotify($uniqid, $payload)));
    }
)->done(
    null,
    function ($error) use ($uniqid) {
        $this->write($this->createLine(Factory::rpcError($uniqid, $error)));
    }
);

WyriHaximus avatar May 15 '19 10:05 WyriHaximus

Hi @WyriHaximus , sure I'll elaborate in the next days

Thanks

MatteoOreficeIT avatar May 20 '19 12:05 MatteoOreficeIT

@MatteoOreficeIT :+1:

WyriHaximus avatar May 20 '19 19:05 WyriHaximus