await() doesn't work with streaming HTTP response body

[bileslav]

Hello,

The code below doesn't work as expected.
At the end, strlen() should return a non-zero.

Am I missing something maybe?

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

React\Async\async(function (): void {
	$browser = new React\Http\Browser();
	$response = $browser->requestStreaming(
		'GET',
		'https://file-examples-com.github.io/uploads/2017/04/file_example_MP4_1920_18MG.mp4',
		[
			'Range' => 'bytes=0-499',
		],
	);

	/** @var Psr\Http\Message\ResponseInterface */
	$response = React\Async\await($response);

	/** @var React\Stream\ReadableStreamInterface */
	$body = $response->getBody();

	$body = React\Promise\Stream\buffer($body);
	$body = React\Async\await($body);

	echo strlen($body), "\n"; // 0
})();

[clue]

@bileslaw Thank you for reporting, this is an interesting one!

I could indeed reproduce the problem you're seeing and have managed to solve it like this:

$browser = new React\Http\Browser();
$promise = $browser->requestStreaming(
    'GET',
    'https://file-examples-com.github.io/uploads/2017/04/file_example_MP4_1920_18MG.mp4',
    [
        'Range' => 'bytes=0-499',
    ],
)->then(function (Psr\Http\Message\ResponseInterface $response) {
    return React\Promise\Stream\buffer($response->getBody());
});

$body = React\Async\await($promise);

echo strlen($body), "\n"; // 0

This example should work just fine. That said, I consider your example to be reasonable and I would have expected this to work as well. I'll keep this ticket open for the reference and will look into addressing this in the underlying await() implementation.

The underlying problem is that await() only resumes in the "next tick" while the HTTP response headers and the body return in the same "same tick". This means once you start awaiting the body buffer, the data has already been emitted (into the void) and the body is already closed.

I've already patched this locally, but will do more tests first. Expect a PR in the next week or so 👍

[bileslav]

@clue Great, thank you!

[WyriHaximus]

@bileslaw Thank you! For reporting this 👍 . Really neat edge case you found there 👍

[clue]

@bileslaw Again thanks for reporting! I've just filed #32 which should address this and make this work as expected, both within an async() call (the easy part) and outside of async() (the hard part). You're looking at 8+ hours of work for #32 alone, plus work in #30 and related tickets. This has definitely been an interesting challenge and I'm glad we've been able to improve this somewhat, enjoy!

[bileslav]

@clue Hello!

I manually replaced SimpleFiber.php with the one from #32 (this), and I can say for sure that something is wrong with it.

It's hard to say what exactly the problem is, but my app is starting to behave incorrectly in various aspects.
Perhaps I was in a hurry (after all, you haven't done merge yet), but I felt it necessary to note this.

[bileslav]

I was able to compose a "minimal" example, here.
The program should print abc abc abc and exit.
After replacing SimpleFiber.php, abc is printed only once.

[bileslav]

Huh, I found that releasing the lock not immediately, but in the next tick, seems to fix this particular problem. I'm a little confused…

[bileslav]

Okay, this cleared things up for me: amphp/sync#22

That is, the error was in my code.
Tomorrow I'll return to the tests with this in mind.