-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ext/web): Prevent TextDecoderStream resource leak on stream cancellation #21074
fix(ext/web): Prevent TextDecoderStream resource leak on stream cancellation #21074
Conversation
Could you add a test case for this? This should be sufficient (it'll trigger the resource sanitizer for tests):
|
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 |
@egfx-notifications FYI as of recently, there is a |
Thank you! That saved me quite some time. I just pushed a version using the 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 |
ea78619
to
38b8e1e
Compare
38b8e1e
to
d2088a8
Compare
I'm done with this unless you request any changes. This may also fix #19946 from how I understand the issue. |
cancel: (_reason) => { | ||
try { | ||
const _ = this.#decoder.decode(); | ||
return PromiseResolve(); | ||
} catch (err) { | ||
return PromiseReject(err); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe something like this can be used to simplify the construction:
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(); | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried to follow the existing style. Not sure why it is preferred here, though.
const file = await Deno.open(filename); | ||
const readable = file.readable.pipeThrough(new TextDecoderStream()); | ||
const chunks = []; | ||
for await (const chunk of readable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test, thanks
This is causing a test flake:
|
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. |
…llation (#21074) 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 Co-authored-by: Bartek Iwańczuk <[email protected]>
…llation (denoland#21074) 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 denoland#13142 Co-authored-by: Bartek Iwańczuk <[email protected]>
…UpOnCancel (denoland#21181) Follow-up fix to denoland#21074
This PR uses the new
cancel
method ofTransformStream
to properly clean up the internalTextDecoder
used inTextDecoderStream
if the stream is cancelled.Fixes #13142