Add ZSTD bindings to the node:zlib package.#6007
Add ZSTD bindings to the node:zlib package.#6007alistairjevans wants to merge 13 commits intocloudflare:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
src/workerd/api/node/zlib-util.c++
Outdated
| ZstdEncoderContext::~ZstdEncoderContext() { | ||
| if (cctx_ != nullptr) { | ||
| ZSTD_freeCCtx(cctx_); | ||
| cctx_ = nullptr; |
There was a problem hiding this comment.
It might be worth wrapping this in a custom smart pointer to ensure the cleanup is correct. Non-blocking tho.
There was a problem hiding this comment.
Added (brotli implementation does already do this)
|
@anonrig ... given your familiarity with the zlib impl in workerd, it would be good to have your review on this as well. |
|
LGTM. @alistairjevans formatter seems to be failing: https://github.com/cloudflare/workerd/actions/runs/21922721360/job/63307147452?pr=6007 @jasnell can you take a look? |
|
Thanks @anonrig, formatting fix pushed, linter passes locally now. |
|
Overall I think this looks good. What I'm considering... (something I wish I had thought to do on the previous |
|
@jasnell, would such a compatibility flag still allow us to opt-in to using the feature in Cloudflare Workers while we wait for the fuzz test to complete? |
|
@alistairjevans ... while the fuzz testing would happening (if we go that route) the compat flag would be marked as "experimental" which means only cloudflare accounts would be able to use it until that annotation is removed. Still need to decide tho if this is the path we'd want to take. Should have a decision soon. |
|
Let's not block - good idea to do as follow up - still want to see this land. |
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
Overall, LGTM with a couple nits and a question about error handling that might need addressing before this lands. I'll do the sign off once the question is looked at :-) .. great job overall!!
There is a pre-existing issue we need to address with CompressionStream::write's validation of flush values but that's not introduced or made worse with this change (it's relevant for the brotli support also). And, there's missing external memory tracking here that can be done as a follow-up (see, for instance, the CompressionAllocator::AllocForBrotli, CompressionAllocator::FreeForZlib, etc). We'll want to come back to that.
src/workerd/api/node/zlib-util.c++
Outdated
| // For decompression, lastResult == 0 means frame is complete | ||
| // If we have flush_ == ZSTD_e_end equivalent and there's input left, that's an error | ||
| if (flush_ == ZSTD_e_end && input_.pos < input_.size && lastResult == 0) { | ||
| // Frame completed but there's still input - could be trailing data |
There was a problem hiding this comment.
Nit: what is this if block for if it's empty?
There was a problem hiding this comment.
This sent me on a bit of a rabbit-hole, initially this code block was added as documentation only, but going back over it made me realise that, unlike the zlib or brotli implementations, you won't get an EOF error if a zstd frame you're decoding got truncated! But you will for brotli and zlib. 😕
Whats even more interesting is that neither will nodejs zstd, which was surprising, a truncated stream will just silently succeed; I'm pretty sure this is a bug in the ZSTD implementation in nodejs, because the tests in https://github.com/nodejs/node/blob/main/test/parallel/test-zlib-truncated.js do not verify zstd behaviour for truncated input as they do with gzip and deflate algorithms, which feels like an oversight.
I've pushed a fairly simple fix for workerd that ensures you do get an EOF error if a payload is truncated. Instinctively I'd go with correctness since this feels like a bug in nodejs, but do say if I should match nodejs behaviour exactly.
There was a problem hiding this comment.
Fixing bugs is good. I'm happy with this and I'd encourage you to open a PR fixing it in node.js also :-)
|
Pushed fixes for linter formatting issues. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6007 +/- ##
==========================================
- Coverage 70.37% 70.35% -0.02%
==========================================
Files 408 408
Lines 108811 109036 +225
Branches 18000 18035 +35
==========================================
+ Hits 76574 76713 +139
- Misses 21430 21495 +65
- Partials 10807 10828 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@alistairjevans Merge executed in #6117. I'll ping you again when it's deployed to prod. |
6 participants
Based pretty heavily on the brotli support.
Closes #4013