framework-x icon indicating copy to clipboard operation
framework-x copied to clipboard

Using await and returning response as promise does not work

Open map-g opened this issue 1 year ago • 2 comments

When using React\Async\await somewhere and then returning the response as a promise (e.g. a promise got from React\Http\Browser::get(), in the example it's replaced by a simple resolve($response)), an assertion in the \FrameworkX\Io\FiberHandler (line 49: assert($response instanceof ResponseInterface);) will fail. After removing this assertion it seems to work fine. Returning the promise without any React\Async\await before does not reveal this.

#!/usr/bin/env php
<?php

declare(strict_types=1);

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

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

$app = new FrameworkX\App();

$app->get('/', function () {
    await(sleep(3));
    $response = React\Http\Message\Response::plaintext(
        "Hello world!\n",
    );
    return resolve($response);
});

$app->run();

map-g avatar Nov 01 '24 12:11 map-g

The issue occurs because you're combining React\Async\await with a Promise returned later in the function. The await() function unwraps the promise but doesn't preserve its "react promise context". This causes the assertion in FrameworkX\Io\FiberHandler to fail because the returned value doesn't conform to expectations.

To fix this, you don't need to use await() in this specific handler. Instead, you should directly chain the sleep() promise and return your response inside the then() callback:

$app->get('/', function () {
    // Use sleep() and properly wrap the response inside the promise chain
    return sleep(3)->then(function () {
        return new React\Http\Message\Response(
            200,
            ['Content-Type' => 'text/plain'],
            "Hello world!\n"
        );
    });
});

This way, you avoid any interference between React\Async\await and promise-handling mechanisms.

The key takeaway is: use sleep() directly when asynchronous behavior is required, and avoid mixing await() with returned promises unless you're certain the environment supports such usage.

panariga avatar Nov 27 '24 16:11 panariga

@map-g Thanks for reporting, while I think we agree that mixing both async styles usually isn't recommended, I also agree that this should be supported in the sense that it should not fail.

Given this is easy to avoid by sticking with either style, I don't consider this high priority and will take a look at this for one of the upcoming milestones.

We're definitely interested in getting this supported and will get back to this when time allows. Let us know if you want to work on this or if you're interested in sponsoring this :+1:

clue avatar Dec 23 '24 13:12 clue