fix(ext/web): Prevent (De-)CompressionStream resource leak on stream cancellation #21199
Conversation
egfx-notifications
force-pushed
the
fix/compressionstream-resource-leak
branch
2 times, most recently
from
0d20541 to
a7fd8f9
Compare
cli/tests/unit/streams_test.ts
Outdated
| await new DecompressionStream("gzip").writable.getWriter().abort(); | ||
| }, | ||
| TypeError, | ||
| "corrupt gzip stream does not have a matching checksum", |
There was a problem hiding this comment.
I don't think we should throw on cancellation. This probably means we need a separate Rust op (or a second parameter) to just throw away the results.
There was a problem hiding this comment.
I actually started work on a separate Rust op, but then decided that we should probably give the user the chance to handle this case as they see fit. Thinking of intentional abortion on the user side where the user wants to continue work with the results received so far, it would be good to let the user know if they need to perform some manual cleanup on the last bytes received or can use the slice as is. This being said, I don't know the specifics of gzip, deflate and deflate-raw enough to say if a decompressed slice is even usable, this being implemented as a stream I suppose it is. If it is, I'd at least give the user the option to request error bubbling, maybe with an options object to the CompressionStream and DecompressionStream constructors.
There was a problem hiding this comment.
I'm not entirely sure here... I might have to hand this one off to someone with a better intuition.
Cancellation on a TextDecoderStream in Firefox appears to reject the read with the cancellation rather than throwing from the writer abort:
(async () => { let td = new TextDecoderStream(); td.writable.abort("hmm"); await td.readable.getReader().read() })()
Promise { <state>: "rejected", <reason>: "hmm" }
I wonder if this is the right way to do things here.
There was a problem hiding this comment.
Yeah, we should follow browser behaviour here (.cancel() should not throw)
cancel() should just close() the underlying resource I think.
There was a problem hiding this comment.
Agreed, I pushed the necessary changes.
ext/web/14_compression.js
Outdated
| @@ -88,6 +91,9 @@ class DecompressionStream { | |||
| const output = ops.op_compression_finish(rid); | |||
| maybeEnqueue(controller, output); | |||
| }, | |||
| cancel: (_reason) => { | |||
| ops.op_compression_finish(rid); | |||
There was a problem hiding this comment.
This could be ops.op_compression_finish(rid, false), and we could use ops.op_compression_finish(rid, true) for the success case right above.
egfx-notifications
force-pushed
the
fix/compressionstream-resource-leak
branch
2 times, most recently
from
5a017c4 to
9c096c2
Compare
|
Just added two more tests to verify that my changes did not break |
…stream cancellation
…eak on stream cancellation
mmastrac
force-pushed
the
fix/compressionstream-resource-leak
branch
from
9c096c2 to
cd53e0b
Compare
mmastrac
force-pushed
the
fix/compressionstream-resource-leak
branch
from
cd53e0b to
87ac9b7
Compare
Based on #21074 and #20741 I was looking for further potential use cases of
TransformStreamcancel()method, so here goCompressionStreamandDecompressionStream.Fixes #14212