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

Gzip/Brotli compressions on NGINX Unit #787

Open
nailarch opened this issue Nov 17, 2022 · 10 comments · May be fixed by #1368
Open

Gzip/Brotli compressions on NGINX Unit #787

nailarch opened this issue Nov 17, 2022 · 10 comments · May be fixed by #1368
Assignees
Labels
z-enhancement ⬆️ Product Enhancement

Comments

@nailarch
Copy link

Enabling gzip/brotli compression on pure NGINX is relatively simple.
However, I want to enable gzip/brotli compression for NGINX Unit.
I'm not seeing anything on the NGINX Unit configuration man page about how to do the same for NGINX Unit.
Is the option not currently supported?

@nailarch nailarch changed the title How to Enable gzip/brotli compressions on NGINX Unit? How to enable Gzip/Brotli compressions on NGINX Unit? Nov 17, 2022
@travisbell
Copy link

Unit itself, does not support compressing responses.

Depending on your app though, this is something you could do app side. For example, Ruby has a Deflater middleware that one can enable, or, you can put Nginx in front on Unit and have Nginx do the work for you.

@tippexs
Copy link
Contributor

tippexs commented Nov 20, 2022

Hi @nailarch as @travisbell already mentioned currently there is no GZip support in Unit. But this is something on our list of features.

@tippexs tippexs changed the title How to enable Gzip/Brotli compressions on NGINX Unit? Gzip/Brotli compressions on NGINX Unit Nov 20, 2022
@tippexs tippexs added the z-enhancement ⬆️ Product Enhancement label Nov 20, 2022
@andypost
Copy link

Unit can serve static files but there is a trick to provide separate match for each of values from Accept-Encoding

@alejandro-colomar alejandro-colomar linked a pull request Aug 2, 2023 that will close this issue
@alejandro-colomar
Copy link
Contributor

If you're interested in testing some patches that do gzip, there's #914

It would be interesting to see testing on real world workloads and connections, and not just my localhost loops. :)

@ac000 ac000 self-assigned this May 15, 2024
@andypost
Copy link

is this feature still in roadmap? or better to control compression at app-level?

@alejandro-colomar
Copy link
Contributor

alejandro-colomar commented May 20, 2024

I still have my patches around. I found some things that I'd like to polish (e.g., I discovered Unit's numeric parsing functions recently in a different discussion, so I wanted to use those instead of the strtoul(3) wrapper I added in them), and they also need rebasing.

However, I don't work at NGINX anymore, so those patches are bit rotting.

@ac000
Copy link
Member

ac000 commented May 20, 2024

We are working on this feature using @alejandro-colomar patches for inspiration but taking a slightly different approach... watch this space!

@alejandro-colomar
Copy link
Contributor

We are working on this feature using @alejandro-colomar patches for inspiration but taking a slightly different approach... watch this space!

Hi @ac000,

While the Apache 2.0 license certainly allows it, and anyway, I wrote most of that code as part of my employement at F5 (so F5 is the owner of most of the code), I find it somewhat disrespectful to take my patches and re-develop them in a closed-source manner. Would you mind discussing what is being done to those patches in public? How much different is that slightly different approach and why? From what revision of the patches are you taking inspiration?

Cheers,
Alex

@ac000
Copy link
Member

ac000 commented May 20, 2024

Hi @ac000,

While the Apache 2.0 license certainly allows it, and anyway, I wrote
most of that code as part of my employement at F5 (so F5 is the owner
of most of the code), I find it somewhat disrespectful to take my
patches and re-develop them in a closed-source manner. Would you mind
discussing what is being done to those patches in public? How much
different is that slightly different approach and why? From what
revision of the patches are you taking inspiration?

Hi Alex,

As has been discussed in the past, unfortunately your patches are not in
a mergeable state.

After discussing this with @hongzhidao I have decided to re-do this
feature.

The first change is to not introduce a generic filter mechanism, this
needs more thought.

Other differences are

  1. The enabling of the compression feature will initially happen at the
    global settings.http level, with the future ability to do per route
    overrides.

If you have 100's of routes all wanting compression, this will save a
lot of typing and excessive growth of the config file.

  1. Having clear support for multiple compressors from the start;
    deflate, gzip. brotli and zstd

  2. Keeping a clear separation between the actual compressors and Unit,
    making the actual compression code just enough to do the compression
    without having any knowledge of Unit internals will aid the addition of
    the future compressors and reduces complex code duplication.

Well that's the plan anyway, we'll see how it pans out...

As for doing this 'in a closed-source manner', I don;t think this is any
different to anything else, initial development is done usually before
exposing it to the world once you have something that at least works to
show.

Proper attribution will be given where appropriate.

From what revision of the patches are you taking inspiration?

Ah, I see you've removed it now, but yeah it's this
http://www.alejandro-colomar.es/src/alx/nginx/unit.git/commit/?h=gzip

Sorry if you've took offense to any of this...

@alejandro-colomar
Copy link
Contributor

alejandro-colomar commented May 20, 2024

As has been discussed in the past,

Actually, never discussed. I only heard that from you in a recent mail, with no actual discussion, but rather a vague reference to Zhidao's also vague opinion on the patches.

unfortunately your patches are not in a mergeable state.

I agree. There are a few things I wanted to clean up.

After discussing this with @hongzhidao I have decided to re-do this feature.

The first change is to not introduce a generic filter mechanism, this needs more thought.

Hmmm, yeah, could agree. That was the most obscure part of the integration with Unit.

Other differences are

1. The enabling of the compression feature will initially happen at the
   global settings.http level, with the future ability to do per route
   overrides.

If you have 100's of routes all wanting compression, this will save a lot of typing and excessive growth of the config file.

Hmm.

2. Having clear support for multiple compressors from the start;
   deflate, gzip. brotli and zstd

Hmm, I think my implementation clearly supports multiple compressors. The intervention to add more should be minimal, I think. But yeah, if something can be improved in this front, it's good.

3. Keeping a clear separation between the actual compressors and Unit,
   making the actual compression code just enough to do the compression
   without having any knowledge of Unit internals will aid the addition of
   the future compressors and reduces complex code duplication.

Hmm, similar thoughts here (but yeah, I had to deal with those weird buffers; maybe that can be improved by changing Unit internals).

Well that's the plan anyway, we'll see how it pans out...

As for doing this 'in a closed-source manner', I don;t think this is any different to anything else, initial development is done usually before exposing it to the world once you have something that at least works to show.

Well, if some patches are already working and published, the usual thing to do is to say "hey, I don't like X from your patches, will try Y instead".

Proper attribution will be given where appropriate.

Thanks!

From what revision of the patches are you taking inspiration?

Ah, I see you've removed it now,

It's online again.

but yeah it's this http://www.alejandro-colomar.es/src/alx/nginx/unit.git/commit/?h=gzip

The branch was in the middle of a rework. I don't remember its state too well. I'd suggest inspiring on the tag gzip-v37, unless you find the branch better in some aspect.

Sorry if you've took offense to any of this...

I did, but not specifically from you; I appreciate you. ;)

Have a lovely day!
Alex

@ac000 ac000 removed a link to a pull request Jul 23, 2024
@ac000 ac000 linked a pull request Jul 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
z-enhancement ⬆️ Product Enhancement
Projects
Status: 💡Planned
Development

Successfully merging a pull request may close this issue.

6 participants