fix(ext/web): Prevent TextDecoderStream resource leak on stream cancellation

[egfx-notifications]

This PR uses the new cancel method of TransformStream to properly clean up the internal TextDecoder used in TextDecoderStream if the stream is cancelled.

Fixes #13142

[CLAassistant]

All committers have signed the CLA.

[mmastrac]

Could you add a test case for this?

This should be sufficient (it'll trigger the resource sanitizer for tests):

new TextDecoder('utf8').decode(new Uint8Array(), { stream: true });

[egfx-notifications]

Could you add a test case for this?

I can, though I'm not satisfied with the fix yet as this recreates the internal decoder resource for every chunk of the stream. I'm going to look further into the root cause of this, hence the draft status of the PR. I will update this PR and add a test once I have (hopefully) come up with a better solution.

From what I've found out so far the problem is that the decoder from encoding_rs requires a final call of a decode_* function with the last argument set to true. The deno TextDecoder provides the last argument as the negated value of the stream option, so we have to call TextDecoder.decode() at least once without the stream option to free the resource. This is what TextDecoderStream does in its flush method, I'm yet to find the proper way of handling this for an aborted async iterator.

[crowlKats]

@egfx-notifications FYI as of recently, there is a TransformStream cancel option (#20741)

[egfx-notifications]

@egfx-notifications FYI as of recently, there is a TransformStream cancel option (#20741)

Thank you! That saved me quite some time.

I just pushed a version using the cancel option and it works fine for my test case. Feel free to have a look, this is my first deep dive into deno code, so I may be missing something.

I'm going to test this properly and also add a minimal test case sometime next week. The test case suggested by @mmastrac is a bit too minimal as I consider the problem to be a cancelled stream not cleaning up its internal resources. Explicitly calling TextDecoder.decode() with the stream option without actually using it in a stream is not a case I want to solve, especially as the deno test output already mentions how to close the TextDecoder resource properly.

[egfx-notifications]

I'm done with this unless you request any changes.

This may also fix #19946 from how I understand the issue.

[mmastrac]

I believe something like this can be used to simplify the construction:

Suggested change

	cancel: (_reason) => {
	try {
	const _ = this.#decoder.decode();
	return PromiseResolve();
	} catch (err) {
	return PromiseReject(err);
	}
	},
	// deno-lint-ignore require-await
	cancel: async (_reason) => {
	const _ = this.#decoder.decode();
	},

[mmastrac]

I see that this entire class is using the other style -- I'm not sure why so let's leave as-is. This is probably a good follow-up issue. I'll merge it!

[egfx-notifications]

Yes, I tried to follow the existing style. Not sure why it is preferred here, though.

[mmastrac]

Great test, thanks

[mmastrac]

This is causing a test flake:

[text_encoding_test 001.57] textDecoderStreamCleansUpOnCancel => ./cli/tests/unit/text_encoding_test.ts:323:6
[text_encoding_test 001.57] error: Leaking async ops:
[text_encoding_test 001.57]   - 1 async operation to op_read was started in this test, but never completed.
[text_encoding_test 001.57] To get more details where ops were leaked, run again with --trace-ops flag.

[egfx-notifications]

Yeah, just noticed this as well. Thanks for the fixed test. I was going to suggest deferring the sanitizers as suggested here, but your fix seems cleaner.