compress plugin - use configured compression levels for gzip and brotli#12924
compress plugin - use configured compression levels for gzip and brotli#12924JakeChampion wants to merge 1 commit intoapache:masterfrom
Conversation
bc33d15 to
1aeb0d1
Compare
Previously, gzip and brotli compression used hardcoded levels even when custom levels were configured via `HostConfiguration` Split initialization into `data_alloc()` and `transform_init()` so that gzip uses `hc->zlib_compression_level()`, and brotli uses `hc->brotli_compression_level()` and `hc->brotli_lgw_size()` Also adds proper error handling if encoder initialization fails
1aeb0d1 to
f706ecb
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the compress plugin so that gzip and brotli compression use the levels configured via HostConfiguration instead of hardcoded values. The approach mirrors the existing pattern used for zstd compression: splitting initialization into data_alloc() (struct setup) and a new transform_init() (encoder configuration with the configured levels). It also adds proper error handling when encoder initialization fails, replacing fatal() (which calls exit(-1)) with graceful error returns.
Changes:
gzip_compress.cc/h: Extracts encoder initialization fromdata_alloc()into a newtransform_init()that readshc->zlib_compression_level(). Replacesfatal()with error logging + returnfalse.brotli_compress.cc/h: Extracts encoder initialization fromdata_alloc()into a newtransform_init()that readshc->brotli_compression_level()andhc->brotli_lgw_size(). Adds guard indata_destroy()for the newnullptrinitial state.compress.cc: Calls the newGzip::transform_init()andBrotli::transform_init()fromcompress_transform_init(), with error handling andTSHandleMLocReleasecleanup (also fixes a pre-existing missing release in the zstd failure path).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/compress/gzip_compress.cc |
Splits data_alloc() and new transform_init(); uses configured compression level; replaces fatal() with recoverable error |
plugins/compress/gzip_compress.h |
Declares new transform_init() |
plugins/compress/brotli_compress.cc |
Splits data_alloc() and new transform_init(); uses configured level and window size; adds null guard in data_destroy() |
plugins/compress/brotli_compress.h |
Declares new transform_init() |
plugins/compress/compress.cc |
Calls new transform_init() for gzip and brotli; adds TSHandleMLocRelease to all failure paths |
| } | ||
|
|
||
| int compression_level = data->hc->zlib_compression_level(); | ||
| debug("gzip compression context initialized with level %d", compression_level); |
There was a problem hiding this comment.
The debug message "gzip compression context initialized with level %d" is printed at line 74, which is before deflateInit2 is actually called at line 76. If deflateInit2 fails, this message will still have been logged, suggesting the context was initialized when it was not. The debug message should be moved to after the successful deflateInit2 call (and after the optional deflateSetDictionary call), just before return true, to accurately reflect initialization success.
| if (err != Z_OK) { | ||
| fatal("gzip-transform: ERROR: deflateInit (%d)!", err); | ||
| error("gzip-transform: deflateInit2 failed (%d)", err); | ||
| return false; |
There was a problem hiding this comment.
Gzip::data_destroy calls deflateEnd unconditionally, but after this PR's changes, data_alloc no longer calls deflateInit2 — only transform_init does. If transform_init is never called or fails before completing deflateInit2 successfully (the deflateInit2 failure path at line 78-80 returns false without a paired deflateEnd), deflateEnd will eventually be called on an uninitialized z_stream. According to zlib documentation, calling deflateEnd on a stream not initialized by deflateInit/deflateInit2 is undefined behavior. A guard is needed: either track whether deflateInit2 succeeded (e.g., check data->zstrm.state != nullptr, which zlib sets during initialization), or add a boolean flag zstrm_initialized to Data or the gzip module.
| if ((data->compression_type & (COMPRESSION_TYPE_GZIP | COMPRESSION_TYPE_DEFLATE)) && | ||
| (data->compression_algorithms & (ALGORITHM_GZIP | ALGORITHM_DEFLATE))) { | ||
| if (!Gzip::transform_init(data)) { | ||
| error("Failed to configure gzip/deflate compression context"); | ||
| TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When Gzip::transform_init (or Brotli::transform_init) fails and compress_transform_init returns early, data->state has already been set to transform_state_output (line 325). This means compress_transform_do won't call compress_transform_init again, and will proceed to call compress_transform_one and compress_transform_finish, which in turn call Gzip::transform_one / Gzip::transform_finish (or brotli equivalents) on an encoder that was never successfully initialized. This is undefined behavior that can lead to crashes.
The fix should either: (a) change the state to a new "error" state that prevents further processing, or (b) use compress_transform_init's return value to skip subsequent processing in compress_transform_do.
|
Overall looks good. Please address the comments from Copilot. |
Previously, gzip and brotli compression used hardcoded levels even when custom levels were configured via
HostConfigurationSplit initialization into
data_alloc()andtransform_init()sohc->zlib_compression_level()hc->brotli_compression_level()andhc->brotli_lgw_size()Also adds proper error handling if encoder initialization fails