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

Nginx gzip filter compatibility issue #18

Open
brucekirkpatrick opened this issue Jul 11, 2021 · 4 comments
Open

Nginx gzip filter compatibility issue #18

brucekirkpatrick opened this issue Jul 11, 2021 · 4 comments

Comments

@brucekirkpatrick
Copy link

Thank you very much for making this project. I am definitely going to use it for my C++ project to integrate it with Nginx since I found it to be an excellent solution compared to all other things I considered. I found your API functions and documentation excellent and everything worked very quickly and easily.

I did however find a bug in the current version of Nginx 1.21 and this project's master branch which you may want to review.

Basically gzip doesn't work for the urls using this module and I isolated how to fix this.

In ngx_link_func_module.c, there are these 2 lines.
r->headers_out.content_type.len = resp_content_type->len;
r->headers_out.content_type.data = resp_content_type->data;

For whatever reason, ngx_http_code_module.c is checking an addition length variable, which also exists in the nginx documentation, so there is some kind of reason they have this length variable needing to be set twice. This causes this logic to evaluate as true in the gzip module which disables it: ngx_http_test_content_type(r, &conf->types) == NULL

When I add this line next to those, gzip is able to work!

r->headers_out.content_type_len = resp_content_type->len;

You can see this variable is documented here and they say it usually needs to be the same as content_type.len:
https://www.nginx.com/resources/wiki/extending/api/http/

In the process of debugging this, I learned a little bit more about how Nginx works and am less scared to try to work on it now especially since i was able to output debug messages and isolate things reasonably easy, though it did take some hunting to understand this one. It feels more like a bug in nginx to have 2 similar variable names.

Thanks again for creating a simple project that allowed me to dive into Nginx source and be productive on the first day.

@brucekirkpatrick
Copy link
Author

I want to post this to be helpful to anyone finding this thread in case they attempt to test with an text/html content type with UTF-8 charset, because there is a second problem with Nginx's API. If you don't specify the content type length as 9 below, gzip will still fail, because internally nginx is making a hash and then there is no matching hash for this even though text/html works.
The nginx gzip_types directive also does not allow fixing this as far as I can determine. They must have some other code fixing this in other modules since I don't have this problem on my live sites. New modules must have to handle it themselves.

ngx_link_func_write_resp_l(
    ctx,
    200,
    "200 OK",
    strlen("200 OK"),
    "text/html; charset=utf-8",
    9, // we force 9 because nginx doesn't understand how to ignore this part: ; charset=utf-8
    buffer,
    strlen(buffer)
);

@brucekirkpatrick
Copy link
Author

I provided pull request: #19

@Taymindis
Copy link
Owner

Taymindis commented Jul 16, 2021

hi @brucekirkpatrick

Thanks for your finding, but I am not so sure yet whether is a typo bug from NGINX? As I couldn't find any documentation that this headers_out.content_type_len been classified as one of the headers_out family. Maybe I was wrong, as Nginx been developed/researched by various product.


ngx_link_func_write_resp_l(
    ctx,
    200,
    "200 OK",
    strlen("200 OK"),
    "text/html; charset=utf-8",
    sizeof("text/html") - 1, // I will suggested you to use this kind of method to count the len for "Hardcoded string" as this is strongly adivised by open source community
    buffer,
    strlen(buffer)
);

Workaround solution(haven't tested)

  1. You could install the src/http/ngx_http.h into your header path? And you could make that content_type_len updated whenever you need it.

Small note advise, This nginx-link-function is design only for light operation as NGINX is Multi-process single thread IO. And we should not have any heavy process like

  1. Connection on Connection Model
  1. Heavy encrypt and decrypt logic

@brucekirkpatrick
Copy link
Author

brucekirkpatrick commented Jul 16, 2021

Yes, I initially had a problem with performance and errors, but then I made use of aio threads, and made sure my ngx_link_func_call was not able to run more then 1 concurrent request per thread. I used the thread id modulus the max threads to lock on a element in an array of mutex to achieve this. After this, I had 60 times better performance without errors, about half the speed of the fastest nginx will run on that machine. It seems like your project is still the best way. The reason I lock like this is to avoid any additional locks in the application. Each thread has its own memory space to do write operations to keep it as lock-free as possible.

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

No branches or pull requests

2 participants