-
-
Notifications
You must be signed in to change notification settings - Fork 62
[SERVER] Support gzip encoding instead of deflate. #757
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #757 +/- ##
==========================================
+ Coverage 61.09% 61.25% +0.15%
==========================================
Files 58 58
Lines 3789 3802 +13
Branches 2080 2084 +4
==========================================
+ Hits 2315 2329 +14
+ Misses 1473 1472 -1
Partials 1 1
Continue to review full report at Codecov.
|
7e1e9d5
to
b21ad25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our unit tests don't check the correctness of compressed content, did you verify that the code actually works with the browsers?
src/server/response.cpp
Outdated
|
||
#define KIWIX_MIN_CONTENT_SIZE_TO_DEFLATE 100 | ||
|
||
#define KIWIX_MIN_CONTENT_SIZE_TO_COMPRESS 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this threshold is too low - why trying to compress payload that is smaller than the header section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes. It is a pretty old magic value and it seems it has been introduce as some kind of workaround for a segfault kiwix/kiwix-tools@4c79cfc
The question is what would be the good value ? I've seen different values but it seems that MTU is a good limit (https://www.computerworld.com/article/2693941/why-it-doesn-t-make-sense-to-gzip-all-content-from-your-web-server.html). So we can move this to 1400 Bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the code but I didn't build and manually test it (since the unit tests don't exhaustively validate this change). Before merging, this functionality must be tested via kiwix-serve and the browser development console (in order to verify that kiwix-serve sends gzip-compressed content and the browser is able to decompress it).
The `compress` function is copied from httplib
c52a903
to
fba0f09
Compare
Perfect! Why don't you merge this PR then? |
Because I was waiting for CI to pass after my rebase-fixup and switch to something else. |
The
compress
function is copied from httplibFix #753