refactor: php_brotli_stream_data struct #61
Conversation
WalkthroughThis change refactors the management of Brotli encoder and decoder states within the codebase by consolidating multiple separate fields into a unified context structure. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Stream
participant php_brotli_context
User->>Stream: Open Brotli stream
Stream->>php_brotli_context: Initialize context
User->>Stream: Read/Write data
Stream->>php_brotli_context: Use encoder/decoder via context
User->>Stream: Close stream
Stream->>php_brotli_context: Cleanup all resources
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
brotli.c (1)
609-619: 🛠️ Refactor suggestion
⚠️ Potential issue
php_stream_writereturn value is ignored – potential silent data lossInside the flush loop we write compressed data but never verify that the full buffer actually reached the underlying stream. If
php_stream_write()returns <out_size, the remaining bytes are silently discarded.- php_stream_write(self->stream, output, out_size); + if (php_stream_write(self->stream, output, out_size) != out_size) { + php_error_docref(NULL, E_WARNING, + "brotli: short write on compressed stream"); + efree(output); + php_brotli_context_close(&self->ctx); + return EOF; + }Applying the same guard in other write sites (e.g.
php_brotli_compress_write) will make the stream more robust.
♻️ Duplicate comments (1)
brotli.c (1)
670-676: Repeat of uncheckedphp_stream_writeAs in the close routine, the incremental write path should validate that the entire chunk was written. See the earlier diff suggestion for a minimal fix.
🧹 Nitpick comments (4)
brotli.c (4)
545-560: Repeated heap (re)allocations inside hot path – consider reusing buffers
php_brotli_decompress_read()mallocs a freshinputandoutputbuffer for every read call, then frees them a few lines later. In heavy streaming scenarios this hurts performance and fragments the heap.A low-impact optimisation is to promote these two buffers into
self->ctx(similar toctx->outputon the compress side) and reuse them until the stream is closed.
564-565: Minor: renameout_sizeto avoid shadow-ing/confusion
size_t out_size = (size_t)(self->ctx.next_out - output);is clear, but note the same identifier name is used in other scopes (e.g.php_brotli_compress_close). Considerchunk_sizeorwrittenhere to avoid shadow-ing during refactors.
576-579: Zero-byte read edge caseIf
php_stream_read()returns0(EOF or error) we still passinputto Brotli withavailable_in == 0. This is harmless but unnecessary; an early break would avoid an extra decoder round-trip.
777-783: Init path covers zero-initialisation, but ecalloc makes it double work
selfis allocated withecalloc, which already zeroes the memory. Immediately callingphp_brotli_context_init(&self->ctx)repeats the work. This is harmless but redundant.- self = ecalloc(sizeof(*self), 1); - ... - php_brotli_context_init(&self->ctx); + self = ecalloc(sizeof(*self), 1); /* already zeroed */ + /* php_brotli_context_init(&self->ctx); -- not strictly necessary */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
brotli.c(8 hunks)
🔇 Additional comments (5)
brotli.c (5)
480-484: Encapsulation improvement looks goodReplacing the scattered encoder/decoder & buffer fields with one
php_brotli_context ctx;greatly simplifiesphp_brotli_stream_data. The change enhances maintainability and reduces the risk of partial-cleanup leaks because a singlephp_brotli_context_close()now handles all resources.
507-508: Single-point cleanup is correctSwitching to
php_brotli_context_close(&self->ctx)centralises the teardown logic and prevents future drift between close paths. Nicely done.
638-639: Good use of unified teardown
php_brotli_context_close(&self->ctx);here mirrors the decompression path and prevents leaks of the encoder instance or stale output buffer.
781-783: Encoder creation failure path is correctThe new code passes
&self->ctx.encoderwhich guarantees the pointer in the context remains NULL on failure – preventing accidental double free later.
792-796: Symmetric handling for decoder creation looks goodDecoder creation mirrors the encoder path and integrates neatly with the unified context.
Summary by CodeRabbit