cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.29k stars 309 forks source link

🐛 Bug Report — Runtime APIs - WritableStream DefaultWriter await write() does not copy chunk buffer #3137

Open pmeenan opened 6 days ago

pmeenan commented 6 days ago

If you write a buffer to a TransformStream's WritableStream with await writer.write() and then modify the buffer after the promise completes, the output stream will be corrupted.

As best as I can tell from the spec, it should be safe to modify the chunk after the promise resolves:

If chunk is mutable, producers are advised to avoid mutating it after passing it to write(), until after the promise returned by write() settles. This ensures that the underlying sink receives and processes the same value that was passed in.

As it stands currently, the only safe way to do it is to slice() the buffer to make a copy of it when you pass it to write().

This was discovered as part of working on a worker that uses wasm to compress the response streams in a passthrough transform stream. The resulting stream would be corrupted with chunks being overwritten with later data.

jasnell commented 5 days ago

Yes, this is a known limitation of the existing implementation that we end up having to preserve for backwards compatibility :-/ ... The other approach you could take aside from slice() is to copy the input like writer.write(new Uint8Array(chunk)). We've considered adding a fix for this behind a compatibility flag but it wasn't clear if folks were actually bumping up against this or not. I think at this point it's clear we should.

pmeenan commented 5 days ago

Just documenting the behavior difference on the docs would probably be enough if you need to maintain it for compatibility reasons. It's just a pain to debug when it happens.