#21 Make xz-decompress compatible with Cloudflare Workers - #22
#21 Make xz-decompress compatible with Cloudflare Workers#22michaelherger wants to merge 1 commit into
xz-decompress compatible with Cloudflare Workers#22Conversation
|
|
The AI companion identified two major issues when running in Cloudflare's `workerd` runtime: > pull() is being invoked before start()'s promise (mutex acquire + one‑time WebAssembly.instantiate of the embedded wasm blob) has actually resolved and set xzContext. Per the Streams spec, pull() should never fire until start()'s returned promise settles — but that's exactly what's happening here, in your Cloudflare Worker. > Cloudflare doesn't accept dynamic compilation of the WASM. It's expecting it pre-compiled. This change tries to address these two issues without breaking backwards compatibility. Drawback: I believe the 12kB WASM file would become part of the distribution. Signed-off-by: Michael Herger <michael@herger.net>
ec08fee to
07f7001
Compare
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Some notes here.
The other thing is testing: this 100% needs a test setup that validates it. Cloudflare provide Miniflare for this, you'll need to add a CI setup with that. We can probably just run the same test suite but using Miniflare. Should be able to set something up and confirm that it fails in the same way you're seeing today against the current code, and then add this fix and see it start passing.
| const base64Wasm = xzwasmBytes.replace('data:application/wasm;base64,', ''); | ||
| const wasmBytes = Uint8Array.from(atob(base64Wasm), c => c.charCodeAt(0)).buffer; | ||
| return WebAssembly.compile(wasmBytes); | ||
| })().catch(() => null); |
There was a problem hiding this comment.
Compilation still needs to be lazy in all cases
| xzContext.dispose(); | ||
| xzContext = null; | ||
| } | ||
| XzReadableStream._contextMutex.release(); |
There was a problem hiding this comment.
I'm very suspicious of the changes in the stream machinery here. Seems like a totally separate issue, and I suspect the agent has just got confused and run into a separate issue because there's no reason Workers should have different stream issues than normal JS.
Lets drop all of this and do a PR just for module loading. If there is a separate issue with the stream behaviour, we should be able to reproduce it in Node with a failing test, and then fix it independently in another PR. For now lets revert all of that and just fix the module part here.
|
|
||
| ```js | ||
| import { XzReadableStream } from 'xz-decompress'; | ||
| import xzWasmModule from 'xz-decompress/dist/native/xz-decompress.wasm'; |
There was a problem hiding this comment.
This makes that internal path part of the public API contract for this module. It's also a bit of an awkward fallback generally, especially in downstream modules that want to wrap this package.
I have an interesting alternative approach: what if we create a pure JS fallback? We can mechanically compile the existing inline WASM to asm.js with wasm2js and just ship the JS equivalent. Browsers no longer optimize asm nowadays, so it's a bit slower, but it's pure JS so it'll run anywhere. That removes the WASM requirement completely. Performance hit is unlikely to matter unless your app is decompressing huge XZs in batch all day long. Makes usage and deployment way simpler, covers lots of other cases cleanly, and it'd be easy to bring back the optional WASM file approach. Means no special APIs or funky WASM deployment steps required.
I'd suggest we still use the current model by preference, but pull in a precompiled pure JS equivalent when it's unavailable.
Would that work for you?
|
Thanks for your feedback! I put this up to see whether there was some worth in it (from your POV) or not. I actually still have CF's web page about testing worker code open in my browser, as that's something I wanted to add, too. But as it requires its own runtime, I wasn't show how to integrate with the existing testing framework. As I mentioned this change was generated by AI. And I also mentioned that this all was way beyond me... It might be a good learning experience for me just to try to understand all of the feedback you've given so far. But I doubt I can contribute much, as I'm far out of my comfort zone here Do you mind me asking more questions about your comments? I don't want to waste your time. |
3 participants
The AI companion identified two major issues when running in Cloudflare's
workerdruntime:This change tries to address these two issues without breaking backwards compatibility. While I have failed to create tests which would run in Cloudflare's
workerd, I can confirm that the modified module succeeds decompressing file in Cloudflare's cloud.Drawback: I believe the 12kB WASM file would become part of the distribution.