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

Implement Generator-based response streaming #138

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

ivanpepelko
Copy link

@ivanpepelko ivanpepelko commented Mar 14, 2024

POC of proper Response streaming.

I'm currently working on an app which uses Response streaming. It's a simple use case, where progress updates are pushed to the client.
The team uses both symfony-cli runtime and Roadrunner.

With this PR, I'd like to open a discussion on whether this is the correct approach (eg. I'm not sure this extended version of StreamedResponse fits into this project).

IMPORTANT: This will not work without appropriate change in symfony/http-kernel, which wraps Response callback and does not return the value (which must be a Generator instance) (PR).

This update changes the client API.
Before:

use Symfony\Component\HttpFoundation\StreamedResponse;

$response = new StreamedResponse();
$response->setCallback(function (): void {
    echo 'Hello World';
    flush();
    sleep(2);
    echo 'Hello World';
    flush();
});

After:

use Baldinof\RoadRunnerBundle\Http\StreamedResponse;

$response = new StreamedResponse();
$response->setCallback(function (): void {
    yield 'Hello World';
    sleep(2);
    yield 'Hello World';
});

At the moment, in my project I've added composer hook to patch files from the PR. This is not an optimal solution as it adds unnecessary maintenance overhead.

@praswicaksono
Copy link

Hi @ivanpepelko you might want to look this PR: https://github.com/Baldinof/roadrunner-bundle/pull/130/files it seem similar with yours maybe you can take over from there

throw new \LogicException('The Response callback must be set.');
}

foreach ($this->getGenerator() as $chunk) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it compatible with Symfony StreamedResponse by allowing non generator callbacks?

@ivanpepelko
Copy link
Author

To be honest, I'm out of the loop on this issue. In my project, I replaced this library with fluffydiscord/roadrunner-symfony-bundle, which supports response streaming.
Also, not having PR merged on symfony side is a deal breaker for this PR.

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

Successfully merging this pull request may close these issues.

3 participants