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

feat: enable http compression by sending data not with chunked encodi… #691

Merged
merged 2 commits into from
May 4, 2023

Conversation

DeepDiver1975
Copy link
Contributor

@DeepDiver1975 DeepDiver1975 commented May 3, 2023

…ng - if possible

http compression can only be used if chunked encoding is not used.
Therefore if the response body "fits into one chunk" end() is called instead of write() followed by end()

refs: https://openswoole.com/docs/modules/swoole-http-server/configuration#http_compression

Applies to swoole as well

@nunomaduro
Copy link
Member

Can you add tests on this please?

@nunomaduro nunomaduro marked this pull request as draft May 3, 2023 17:09
@DeepDiver1975 DeepDiver1975 force-pushed the feat/http-compression branch 2 times, most recently from 13b7fc1 to 27e6ac3 Compare May 3, 2023 19:06
@DeepDiver1975
Copy link
Contributor Author

Can you add tests on this please?

Two tests have been added:

  • one to test the chunked behavior
  • one to test the not chunked behavior

@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review May 3, 2023 19:24
@taylorotwell
Copy link
Member

Has conflicts.

@taylorotwell taylorotwell marked this pull request as draft May 3, 2023 19:47
@taylorotwell
Copy link
Member

Please mark as ready for review when fixed.

@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review May 3, 2023 20:04
@DeepDiver1975
Copy link
Contributor Author

@taylorotwell conflict resolved - thx

@taylorotwell taylorotwell merged commit 2568cc3 into laravel:1.x May 4, 2023
@DeepDiver1975 DeepDiver1975 deleted the feat/http-compression branch May 4, 2023 15:11
@DeepDiver1975
Copy link
Contributor Author

THX @taylorotwell

@alexryall
Copy link

alexryall commented May 23, 2023

This change contains a breaking change for us. Currently we are GZIP encoding responses with the below code and with this change the server returns malformed responses. For now I've locked our Octane version to 1.5.4, what would be the recommended way to GZIP encode responses? I don't want to add an extra layer e.g. Nginx just to encode the response.

<?php

namespace App\Http\Middleware;

use Closure;
use Illuminate\Http\Request;

class GzipEncodeResponse
{
    /**
     * @param Request $request
     * @param Closure $next
     * @return \Illuminate\Http\Response|\Illuminate\Http\JsonResponse
     */
    public function handle(Request $request, Closure $next)
    {
        $response = $next($request);

        if (in_array('gzip', $request->getEncodings()) && function_exists('gzencode')) {
            $response->setContent(gzencode($response->getContent(), 5));
            $response->headers->add([
                'Content-Encoding' => 'gzip',
            ]);
        }

        return $response;
    }
}

@DeepDiver1975
Copy link
Contributor Author

DeepDiver1975 commented May 23, 2023

swoole has built in compression capabilities - see the link in the description of this pr.

as long as compiled with the correct options it will work out of the box and there is no need for extra code

@alexryall
Copy link

@DeepDiver1975 thanks for explaining, that makes sense. I did try to get the automatic compression working with the official Swoole docker image (using this one currently) but didn't have any joy so ended up doing it within Laravel. I thought it was probably a combination of http_compression not being set to true within Octane and the way the Docker image has been setup.

@alexryall
Copy link

With a bit of messing around managed to get Brotli and Gzip working thanks to this change. You're right about the correct packages (libz/brotli) needing to be installed before Swoole is installed.

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.

4 participants