Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

await() error in SimpleFiber.php #16

Closed
szado opened this issue Nov 29, 2021 · 17 comments · Fixed by #26
Closed

await() error in SimpleFiber.php #16

szado opened this issue Nov 29, 2021 · 17 comments · Fixed by #26
Labels
help wanted Extra attention is needed question Further information is requested
Milestone

Comments

@szado
Copy link

szado commented Nov 29, 2021

(Great job with Fibers initiative!)
For test purposes I'm refactoring my app with await() function in place of promises-chains. I've got an error:

2021-11-29 21:56:23 [EXCEPTION] Error: Value of type null is not callable in ...\vendor\react\async\src\SimpleFiber.php:59
2021-11-29 21:56:23 [EXCEPTION] #0 ...\vendor\react\async\src\functions.php(92): React\Async\SimpleFiber->suspend()
2021-11-29 21:56:23 [EXCEPTION] #1 ...\MessagesProcessor.php(114): React\Async\await(Object(React\Promise\Promise))

It occurs when I try to use async() function. It's hard to say how to reproduce this bug. In the same call stack, a few promises before, I use a React\Http\Browser with async() function. If I comment this usage (replace to something like resolve(new ResultObject())) - error doesn't occur. If I use Browser with promise-style api, the PHP process ends with error:

Process finished with exit code -1073741819 (0xC0000005)

I'm using the newest versions of all packages, ofc PHP 8.1.
Do you know what can cause these errors?

@szado
Copy link
Author

szado commented Nov 30, 2021

This code causes Segmentation fault error (CLI on Windows):

<?php
require 'vendor/autoload.php';
$browser = new \React\Http\Browser();
/** @var \Psr\Http\Message\ResponseInterface $response */
$response = \React\Async\await($browser->get('https://github.com'));
var_dump((string)$response->getBody());

...but only for "github.com", for "google.com" returns proper result.
This works fine in both cases:

<?php
require 'vendor/autoload.php';
$browser = new \React\Http\Browser();
$browser
    ->get('https://github.com')
    ->then(fn (\Psr\Http\Message\ResponseInterface $response) => var_dump((string)$response->getBody()));

EDIT:
Problem exists only on Windows PHP compilation - on linux all works correctly.
EDIT2:
Please check it for yourself on your environment, on my it's not predictable. Yesterday it worked, now it doesn't - in CLI I have segmentation fault.

@clue clue added help wanted Extra attention is needed question Further information is requested labels Dec 4, 2021
@clue
Copy link
Member

clue commented Dec 4, 2021

@szado Thank you for reporting, this definitely shouldn't have happened!

I've tried reproducing this a number of times, but can not reproduce the problem you're seeing on Linux.

A segmentation fault is not something that userland PHP code should be able to trigger. There is also no platform-specific code in ReactPHP involved, so I can only suspect this is an issue with the fibers implementation in PHP itself. I don't think this boils down to an issue in ReactPHP itself, so I wonder if we can break this down to a reproducible script that also shows this segmentation fault without ReactPHP?

Accordingly, it looks like this should probably be reported upstream in PHP. Can anybody take a look at this and link/report back here?

Thank you!

@szado
Copy link
Author

szado commented Dec 5, 2021

@clue maybe this code will be helpful: https://github.com/szado/reactphp-connection-pool/blob/master/test.php
It triggers Segmentation fault and Error: Value of type null is not callable in ...\reactphp-connection-pool\vendor\react\async\src\SimpleFiber.php on line 59 (it can be more directly related to ReactPHP) on Windows and Linux.

@Revalto
Copy link

Revalto commented Dec 5, 2021

Tested on both Windows and Linux, I didn't notice any difference. The error is present both there and there.

Here is my piece of code. I'm trying to wait for a WebSocket response and give it back. As a result, I get an error. If you don't wait for an answer, but just give out any value, everything goes well

return new Promise(function (callable $resolve) {
  ReplyRoute::reply('Server:GetValue', function (int $value) use ($resolve) {
      $resolve($value); // Error
  });

  $resolve(100); // Good
});

@WyriHaximus
Copy link
Member

@szado I can reproduce that error and looking into the exact cause (probably related to PHP Fatal error: Uncaught TypeError: Szado\React\ConnectionPool\ConnectionPool::retryWithDelay(): Return value must be of type Szado\React\ConnectionPool\ConnectionAdapters\ConnectionAdapterInterface, null returned) and then work on a fix.
The browser segfault I can't reproduce on windows. But might be the same root cause internally.

@bartvanhoutte
Copy link

Ran into the same error.

<?php

use React\Promise\Promise;
use React\Promise\PromiseInterface;

use function React\Async\await;
use function React\Promise\resolve;

require_once __DIR__ . '/vendor/autoload.php';

function test(): PromiseInterface {
    $resolver = function ($resolve, $reject) {
      await(resolve());

      $resolve();
    };

    return new Promise($resolver);
}

await(test());
[docker://fiber:latest/]:php /opt/project/index.php

Fatal error: Uncaught Error: Value of type null is not callable in /opt/project/vendor/react/async/src/SimpleFiber.php on line 59

Error: Value of type null is not callable in /opt/project/vendor/react/async/src/SimpleFiber.php on line 59

Call Stack:
    0.0002     391640   1. {main}() /opt/project/index.php:0
    0.0886    4169192   2. React\Async\await($promise = class React\Promise\Promise { private $canceller = NULL; private $result = NULL; private $handlers = [0 => class Closure { virtual $closure = "React\Promise\Promise::React\Promise\{closure}", ... }]; private $progressHandlers = [0 => class Closure { virtual $closure = "React\Promise\Promise::React\Promise\{closure}", ... }]; private $requiredCancelRequests = 0; private $cancelRequests = 0 }) /opt/project/index.php:19
    0.0886    4174816   3. React\Async\SimpleFiber->suspend() /opt/project/vendor/react/async/src/functions.php:89

@WyriHaximus
Copy link
Member

TL;DR You're only having one fiber (the main fiber) running and you want to await for a promise twice, it's simply not possible.

@WyriHaximus
Copy link
Member

P.S. Will get you a longer response somewhere between tonight and the end of the weekend. Working on a bunch of examples and a blog post on this.

@bartvanhoutte
Copy link

@WyriHaximus 👌 It might lead to some unexpected errors when you're using a 3rd party library. Right now the example is pretty useless but replace await(resolve()) with some 3rd party function that undirectly calls await somewhere and you're stuck. Will try to find a workaround in my case.

@bartvanhoutte
Copy link

Something like this instead?

<?php

use React\Promise\PromiseInterface;

use function React\Async\async;
use function React\Async\await;
use function React\Promise\resolve;

require_once __DIR__ . '/vendor/autoload.php';

function test(): PromiseInterface {
    return async(function () {
      await(resolve());
    });
}

await(test());

@WyriHaximus
Copy link
Member

@bartvanhoutte yes, each async will create a new fiber.

@WyriHaximus
Copy link
Member

Just filed #18 that helps with a few cases mentioned in this issue. And intentionally all of them (except the segfault), but I'd have to test that

@WyriHaximus
Copy link
Member

Need to translate this into documentation, but write my thoughts and the outlines of our current plan down at: https://blog.wyrihaximus.net/2021/12/async-and-await-at-the-edge-with-reactphp/

@bartvanhoutte
Copy link

A valid reason I can think of to call await in the main fiber is to determine the exit code of the application. Note that the order of things added to the event loop is important as described in @WyriHaximus's blog.

<?php

use React\EventLoop\Loop;

use function React\Async\async;
use function React\Async\await;

require_once __DIR__ . '/vendor/autoload.php';

$timer = Loop::addPeriodicTimer(1, fn() => print 'tick' . PHP_EOL);

$exit = await(async(function () use ($timer) {
    Loop::addTimer(10, fn() => Loop::cancelTimer($timer));
    return 1;
}));

exit($exit);

@bartvanhoutte
Copy link

bartvanhoutte commented Dec 15, 2021

I can no longer reproduce the error from #16 (comment), however there seems to be an issue with timers using async.

<?php

use React\EventLoop\Loop;

use function React\Async\async;
use function React\Async\await;

require_once __DIR__ . '/vendor/autoload.php';

function foo()
{
    print __FUNCTION__ . PHP_EOL;

    await(async(fn() => null));
}

Loop::addTimer(5, fn() => foo());
[docker://fiber:latest/]:php /opt/project/index.php
foo
foo

Fatal error: Uncaught Error: Value of type null is not callable in /opt/project/vendor/react/async/src/SimpleFiber.php on line 59

The timer is executed twice causing the error.

Upon further inspection, it looks like the timer is never removed from the list of timers in \React\EventLoop\Timer\Timers.

@Provoker
Copy link

The same error occurs when an EventLoop is created manually without using \React\EventLoop\Loop::get
SimpleFiber itself creates a new loop in Loop::run(), resulting in two loops.
https://github.com/reactphp/async/blob/main/src/SimpleFiber.php#L44

Example:

<?php

$loop = new \React\EventLoop\ExtEventLoop();

//\React\EventLoop\Loop::set($loop); // solution

$deferred = new \React\Promise\Deferred();
$loop->futureTick(static fn() => $deferred->resolve());

\React\Async\await($deferred->promise());
$loop->run();

Result: Error: Value of type null is not callable in /vendor/react/async/src/SimpleFiber.php:53

After uncommenting Loop::set the problem is solved.
But \React\EventLoop\Loop::set is marked as @internal, so this solution is not safe.

@clue
Copy link
Member

clue commented Jan 25, 2022

@bartvanhoutte Very good input, I can see where you're coming from! It looks like most of the issues discussed in this ticket stem from lack of documentation how to use the async() and await() functions, so I've just filed #26 to add appropriate documentation. The gist is that making your example work is as simple as updating it like this (refs #19):

<?php

use React\EventLoop\Loop;

use function React\Async\async;
use function React\Async\await;

require_once __DIR__ . '/vendor/autoload.php';

function foo()
{
    print __FUNCTION__ . PHP_EOL;

-    await(async(fn() => null));
+    await(async(fn() => null)()); // or omit as this is a NO-OP
}

- Loop::addTimer(5, fn() => foo());
+ Loop::addTimer(5, async(fn() => foo())); // or Loop::addTimer(5, async('foo'));

@Provoker Interesting find! The await() function indeed assumes it's executing the default loop, so you should simply remove any references to an explicit loop instance. See also https://github.com/reactphp/event-loop#loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants