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

Response doesn't support stream-based output #109

Closed
albertcht opened this issue Apr 7, 2021 · 16 comments
Closed

Response doesn't support stream-based output #109

albertcht opened this issue Apr 7, 2021 · 16 comments

Comments

@albertcht
Copy link
Contributor

albertcht commented Apr 7, 2021

  • Octane Version: 0.1.0
  • Laravel Version: 8.36.1
  • PHP Version: 8.0.3
  • Database Driver & Version:

Description:

Most of PHP's output usages like echo, var_dump output its content to buffer. In Swoole's case the result will print on console instead of web.

It seems this partial code in SwooleClient.php refers from here

protected function sendResponseContent(Response $response, SwooleResponse $swooleResponse): void
{
if ($response instanceof StreamedResponse && property_exists($response, 'output')) {
$swooleResponse->end($response->output);
return;
} elseif ($response instanceof BinaryFileResponse) {
$swooleResponse->sendfile($response->getFile()->getPathname());
return;
}
$content = $response->getContent();
if (strlen($content) <= 8192) {
$swooleResponse->end($content);
return;
}
foreach (str_split($content, 8192) as $chunk) {
$swooleResponse->write($chunk);
}
$swooleResponse->end();
}
because the output property doesn't exist in symfony's response object. The output property is set from here in swooletw/laravel-swoole.

Symfony\Component\HttpFoundation\StreamedResponse doesn't support getContent and setContent for setting output content, so I fetch buffer output and set the result to response's output property manually.

If Octane is going to support stream output, it needs to use ob_ functions to catch stream content as well.

Many thanks to the official team for integrating Swoole into Laravel!

Steps To Reproduce:

echo 'hello world';

var_dump('Swoole is awesome');

response()->streamDownload(function () {
    echo file_get_contents("https://github.com/laravel/laravel/blob/8.x/README.md");
    }, 'laravel-readme.md');
});
@albertcht albertcht changed the title Response doesn't support streaming-based content output Response doesn't support stream-based content output Apr 7, 2021
@albertcht albertcht changed the title Response doesn't support stream-based content output Response doesn't support stream-based output Apr 7, 2021
@themsaid
Copy link
Member

themsaid commented Apr 7, 2021

@albertcht can you open a PR to show the changes needed?

@albertcht
Copy link
Contributor Author

@albertcht can you open a PR to show the changes needed?

Sure, I will submit a PR to show the changes needed for stream-based output.

@27pchrisl
Copy link

27pchrisl commented Apr 8, 2021

It would be ideal if there were a way to access the current Swoole\Http\Response, so that I could call write() on it.

My use case is streaming large JSON objects, which currently is done using a JSON generator calling echo within the StreamResponse::sendContent callback. These objects are too large to hold in memory, so a way of doing this that doesn't involve output buffering would be great!

I'd be happy to assist with a PR, but I wasn't sure where architecturally in Octane this would go.

Do any of:

  • via the Octane facade
  • the dependency injection container
  • by extending StreamResponse
  • via a new interface on an application class that extended StreamResponse

make sense?

@barryvdh
Copy link
Contributor

barryvdh commented Apr 8, 2021

In #138 this was referenced; https://github.com/hhxsv5/laravel-s/blob/master/src/Swoole/DynamicResponse.php Does that help?

@27pchrisl
Copy link

Hi @barryvdh, yes lines https://github.com/hhxsv5/laravel-s/blob/master/src/Swoole/DynamicResponse.php#L24-L26 do work to capture the output from sendContent and write it to the swoole output. But only for payloads on the small side because all the output is being buffered in memory.

The reason an application would choose to use Symfony's StreamedResponse is to avoid any output buffering memory constraint, otherwise you could use the simpler Response.

I think the most robust technique would allow the app to call a write method on a child of Response object which ultimately will call $swooleResponse->write(), but buffers and splits only that specific write to ensure it's less than 2MB. So avoiding buffering the entire stream.

Something along the lines of: https://gist.github.com/27pchrisl/6dbe0993b2423531b4afaf406a05224d

In this example if you were an Octane-aware app that really needs streaming, you'd be pointed at replacing your StreamedResponse with OctaneResponse, and changing your echo calls to $response->emit() calls. Just before calling sendContent, Laravel\Octane\Swoole\SwooleClient::sendResponseContent can then call $response->setSwooleResponse($swooleResponse) if the $response is an OctaneResponse. If the response is not an OctaneResponse but it is a StreamedResponse, then you can buffer the whole lot as in that referenced example for backwards compatibility with existing StreamedResponses that aren't too large.

I'm not sure how Roadrunner works internally, but StreamedResponse seems to emit the data fine on that platform (although I haven't tested how it buffers the output or uses memory for that).

@albertcht
Copy link
Contributor Author

albertcht commented Apr 9, 2021

Hi @27pchrisl,

Indeed, the buffer fetching solution doesn't cover large output case. I am also thinking if there's a way of being compatible with non-Swoole environment in large stream output.

My PR will handle the most buffer cases first, and then try to take care of large output later. More discussions about that are welcomed.

@barryvdh
Copy link
Contributor

barryvdh commented Apr 9, 2021

Basis case is here: #151
But obviously it does not support real streaming, as we wait for the output before we pass it to Swoole.

@barryvdh
Copy link
Contributor

barryvdh commented Apr 9, 2021

I think it's not really easy to capture the output non-blocking. You can provide a callback to ob_start, but I guess if you actually want to use a streaming response in swoole, you would need to create a StreamResponse which uses the SwooleResponse as callback parameter, so you can just do $swooleResponse->write(...) instead of echo ... Not sure for RoadRunner though, because that converts everything to PSR-7.

@albertcht
Copy link
Contributor Author

I think it's not really easy to capture the output non-blocking. You can provide a callback to ob_start, but I guess if you actually want to use a streaming response in swoole, you would need to create a StreamResponse which uses the SwooleResponse as callback parameter, so you can just do $swooleResponse->write(...) instead of echo ... Not sure for RoadRunner though, because that converts everything to PSR-7.

Indeed, haven't come up with a good idea for handling that without providing swoole response instance to stream response.

@barryvdh
Copy link
Contributor

barryvdh commented Apr 9, 2021

You can provide a callback to ob_start, but I guess if you actually want to use a streaming response in swoole, you would need to create a StreamResponse which uses the SwooleResponse as callback parameter, so you can just do $swooleResponse->write(...) instead of echo ..

Whoops, by default it's not chunked. So the 2nd param to ob_start needs to be > 0. Luckily @themsaid fixed that in #153 :)

@27pchrisl
Copy link

I think it's not really easy to capture the output non-blocking. You can provide a callback to ob_start, but I guess if you actually want to use a streaming response in swoole, you would need to create a StreamResponse which uses the SwooleResponse as callback parameter, so you can just do $swooleResponse->write(...) instead of echo ... Not sure for RoadRunner though, because that converts everything to PSR-7.

I think because true streaming responses are a bit specialist, and this OctaneResponse now exists in #153 (which solves the immediate issue), it would be justified to put the onus on the app to have to switch to OctaneResponse from StreamedResponse and call OctaneResponse::write(). It certainly sounds like there's no cunning Swoole-compatible alternative to output buffering an existing StreamedResponse, and that's just the way it is for applications that want to do both streaming and Octane.

To ensure the OctaneResponse class will exist, is it reasonable for a Laravel package to have a dependency on laravel/octane ?

@barryvdh
Copy link
Contributor

barryvdh commented Apr 9, 2021

Well I was wrong because if you use a chunk parameter with the ob start callback, it does flush correctly to the Swoole Response. See the comments on the PR.

@albertcht
Copy link
Contributor Author

Well I was wrong because if you use a chunk parameter with the ob start callback, it does flush correctly to the Swoole Response. See the comments on the PR.

That's cool! I didn't notice there's a 2nd param for that purpose.

@27pchrisl
Copy link

Had to look into it - php/php-src@888f376
Another one of those 21 year old features I've never noticed ; )

Thanks @barryvdh !

@alecpl
Copy link

alecpl commented Feb 1, 2023

I'm "streaming" really big files, like > 4GB in size. And it fails at some point because it looks like Swoole is buffering the whole output and at some point it has not enough memory. Any solution to this?

@alecpl
Copy link

alecpl commented Feb 1, 2023

Oh, I get it. It's SwooleClient::sendResponseContent() responsible for the buffering, not Swoole. Any workarounds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants