octane icon indicating copy to clipboard operation
octane copied to clipboard

Streamed response gets buffered and returned in one chunk

Open danharrin opened this issue 1 year ago • 11 comments

Octane Version

2.3.12

Laravel Version

11.9.2

PHP Version

8.3.0

What server type are you using?

Roadrunner

Server Version

2024.1.2

Database Driver & Version

No response

Description

When streaming a response from a route behind Octane, the response gets buffered and arrives all at once instead of as it is generated.

Currently, wire:stream in Livewire is completely broken when using Laravel Octane, but the issue is not specific to Livewire and my instructions below use native Laravel and vanilla JavaScript only.

Steps To Reproduce

I have created a minimal reproduction repository. Follow the instructions in the README to observe the behaviour of artisan serve as opposed to Octane. There is a small amount of JavaScript in welcome.blade.php that sends the request and outputs the chunks in real time.

Here is the response, note the sleep(1) calls to simulate an artificial delay in receiving the stream chunks. When using artisan serve, there is a second delay between each number appearing. When using Octane, there is a 5 second delay before all numbers appear at once.

return response()->stream(function () {
    echo 1;

    ob_flush();
    flush();
    sleep(1);

    echo 2;

    ob_flush();
    flush();
    sleep(1);

    echo 3;

    ob_flush();
    flush();
    sleep(1);

    echo 4;

    ob_flush();
    flush();
    sleep(1);

    echo 5;
}, 200, [
    'Content-Type' => 'text/html; charset=utf-8;',
    'Cache-Control' => 'no-cache',
    'X-Accel-Buffering' => 'no',
]);

danharrin avatar Jun 04 '24 11:06 danharrin

@barryvdh do you happen to have any insights on this?

Thought you might have any ideas as just found you previously solved StreamedResponse stuff in #151.

gazzoy avatar Jun 04 '24 11:06 gazzoy

Yeah well the 'fix' was to wait until the stream was finished. At that time I didn't find a way to pass the actual stream.

barryvdh avatar Jun 04 '24 15:06 barryvdh

@barryvdh Thanks you! Ok, so seems like streaming w/ Octane is the darkside of the force...

gazzoy avatar Jun 05 '24 00:06 gazzoy

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

github-actions[bot] avatar Jun 05 '24 02:06 github-actions[bot]

RoadRunner does not seem to work well with buffers and uses Generators. Maybe Octane could support Generators?

return response()->stream(function (): Generator {
        yield '1';
        sleep(1);
        yield '2';
        sleep(1);
        yield '3';
    }, 200, [
        'Content-Type' => 'text/html; charset=utf-8;',
        'Cache-Control' => 'no-cache',
        'X-Accel-Buffering' => 'no',
    ]);

RoadRunnerClient:

/**
     * Send the response to the server.
     */
    public function respond(RequestContext $context, OctaneResponse $octaneResponse): void
    {
        if ($octaneResponse->outputBuffer &&
            ! $octaneResponse->response instanceof StreamedResponse &&
            ! $octaneResponse->response instanceof BinaryFileResponse) {
            $octaneResponse->response->setContent(
                $octaneResponse->outputBuffer.$octaneResponse->response->getContent()
            );
        }

        if ($octaneResponse->response instanceof StreamedResponse) {
            $this->client->getHttpWorker()->respond(
                $octaneResponse->response->getStatusCode(),
                $octaneResponse->response->getCallback()(), // Passing here the Generator
                $this->toPsr7Response($octaneResponse->response)->getHeaders()
            );
        } else {
            $this->client->respond($this->toPsr7Response($octaneResponse->response));
        }
    }

this works with RoadRunner.

donnysim avatar Jun 06 '24 17:06 donnysim

RoadRunner does not seem to work well with buffers and uses Generators. Maybe Octane could support Generators?

return response()->stream(function (): Generator {
        yield '1';
        sleep(1);
        yield '2';
        sleep(1);
        yield '3';
    }, 200, [
        'Content-Type' => 'text/html; charset=utf-8;',
        'Cache-Control' => 'no-cache',
        'X-Accel-Buffering' => 'no',
    ]);

RoadRunnerClient:

/**
     * Send the response to the server.
     */
    public function respond(RequestContext $context, OctaneResponse $octaneResponse): void
    {
        if ($octaneResponse->outputBuffer &&
            ! $octaneResponse->response instanceof StreamedResponse &&
            ! $octaneResponse->response instanceof BinaryFileResponse) {
            $octaneResponse->response->setContent(
                $octaneResponse->outputBuffer.$octaneResponse->response->getContent()
            );
        }

        if ($octaneResponse->response instanceof StreamedResponse) {
            $this->client->getHttpWorker()->respond(
                $octaneResponse->response->getStatusCode(),
                $octaneResponse->response->getCallback()(), // Passing here the Generator
                $this->toPsr7Response($octaneResponse->response)->getHeaders()
            );
        } else {
            $this->client->respond($this->toPsr7Response($octaneResponse->response));
        }
    }

this works with RoadRunner.

@donnysim I am just curious. Is what you posted here a complete fix for getting Streamed Responses to work in Octane RoadRunner? Or does more change need to be done to support Generators fully as you suggest?

Orrison avatar Jun 11 '24 17:06 Orrison

@Orrison technically would still need to check if it's a generator for edge cases but it's all you need for it to work with RoadRunner as it already accepts a Generator|string.

donnysim avatar Jun 11 '24 17:06 donnysim

@Orrison technically would still need to check if it's a generator for edge cases but it's all you need for it to work with RoadRunner as it already accepts a Generator|string.

That's amazing. Do you plan on opening a PR for this into Octane?

Would you like any assistance in doing so or help testing?

Orrison avatar Jun 11 '24 17:06 Orrison

@crynobone would y'all consider @donnysim's suggestion of using Generators here?

Orrison avatar Jun 11 '24 17:06 Orrison

I believe @danharrin wanted a stab at a PR as I'm not too time-free to invest time into PR's at the moment. If he decides otherwise then anyone is free maybe to attempt one, or I might as the week or next goes if freed up more. On the side note, when inspecting this I was thinking if it would be possible to convert the buffer into a Generator while maybe catching PHP_OUTPUT_HANDLER_START and PHP_OUTPUT_HANDLER_FINAL etc., but all the clients differ wildly in how they work that there's really no way I'd have time to analyze them all to find if that's a plausible approach.

donnysim avatar Jun 11 '24 18:06 donnysim

I believe @danharrin wanted a stab at a PR as I'm not too time-free to invest time into PR's at the moment. If he decides otherwise then anyone is free maybe to attempt one, or I might as the week or next goes if freed up more. On the side note, when inspecting this I was thinking if it would be possible to convert the buffer into a Generator while maybe catching PHP_OUTPUT_HANDLER_START and PHP_OUTPUT_HANDLER_FINAL etc., but all the clients differ wildly in how they work that there's really no way I'd have time to analyze them all to find if that's a plausible approach.

No problem at all! I totally understand. I just wanted to check with you if you had already planned on doing so. But since you are busy I can see if we and/or @danharrin can take a crack at it. I wanted to thank you for your research and work on this! If we can get StreamedResponses to work in Octane it would fill a massive gap in Octane right now.

Orrison avatar Jun 11 '24 18:06 Orrison

Was anyone still working on a PR for this?

driesvints avatar Jul 02 '24 18:07 driesvints

@driesvints Yes, we believe we have found a fix for this. We overrode the Octane classes in our project as we needed the fix as quickly as possible.

https://github.com/canyongbs/advisingapp/pull/823

We can open a PR to Octane soon with the change.

Orrison avatar Jul 02 '24 18:07 Orrison

@driesvints, just an update on this one. We have planned to open the PR for this in this current Sprint. So, either this week or early next week.

Orrison avatar Jul 08 '24 23:07 Orrison

Thanks @Orrison

driesvints avatar Jul 09 '24 10:07 driesvints

@Orrison did you get to that?

driesvints avatar Jul 23 '24 10:07 driesvints

Hey @driesvints, I'm sorry for the delay. It is in our Sprint, but we had some other items we need to finish up first. We should have the PR soon.

Orrison avatar Jul 23 '24 15:07 Orrison

PR opened: #939

danharrin avatar Aug 02 '24 17:08 danharrin

@danharrin So just to be clear, this PR only fixed the Roadrunner side, other servers still collect the buffer, thus still incompatible with Livewrie's wire:stream?

Lumethys avatar Aug 31 '24 16:08 Lumethys

@Lumethys yes, this is only for Roadrunner, but afaik it will not fix Livewire with Roadrunner as the fix works only when using Generators, thus still invalid for buffered output and I believe Roadrunner will never work with buffers. At the time during the testing Franken did not work with buffers but released and update that fixed it, but I'm not sure if anything changed up till now and I remember buffers worked with Swoole. I do not know if they work in Livewire context tho as the testing was based on returning a stream response.

donnysim avatar Aug 31 '24 18:08 donnysim

Looks like this fix only applied to Roadrunner and there aren't any other open issues. Are there any plans to get this working with Swoole and FrankenPHP?

binaryfire avatar Oct 15 '24 10:10 binaryfire