Skip to content
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

[CP] [vm] Fix leakage of file system watcher related data structures #52793

Closed
srawlins opened this issue Jun 26, 2023 · 4 comments
Closed

[CP] [vm] Fix leakage of file system watcher related data structures #52793

srawlins opened this issue Jun 26, 2023 · 4 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve

Comments

@srawlins
Copy link
Member

Commit(s) to merge

3dd9389

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/311521

Issue Description

Without this fix, there is a bad leak of _Closure objects when using overlapping Directory.watch streams. This change switches a Stream.pipe usage to Stream.listen, and cancels subscriptions.

What is the fix

The fix is to change how these watch events are listened to, and canceling subscriptions.

Why cherry-pick

This memory leak can be very prevalent in the Dart analyzer, and memory usage (including leaks) is a top issue for users of Dart analyzer.

Risk

low

Issue link(s)

#52703

Extra Info

No response

@srawlins srawlins added the cherry-pick-review Issue that need cherry pick triage to approve label Jun 26, 2023
@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Jul 5, 2023
@a-siva
Copy link
Contributor

a-siva commented Jul 5, 2023

lgtm

@mraleph
Copy link
Member

mraleph commented Jul 6, 2023

We might want to cherry pick 8b6e396 as well.

@itsjustkevin
Copy link
Contributor

We might want to cherry pick 8b6e396 as well.

@srawlins should we also be picking the commit @mraleph mentioned above before merging this cherry-pick?

@srawlins
Copy link
Member Author

should we also be picking the commit @mraleph mentioned above before merging this cherry-pick?

8b6e396 was landed recently, so we would want to land this (3dd9389) cherry-pick first. After a quick evaluation, cherry-picking 8b6e396 helps with this memory theme as well.

@itsjustkevin itsjustkevin added cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. labels Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve
Projects
None yet
Development

No branches or pull requests

8 participants