Skip to content

Awaiting a static variable Future in async/await closure leaks too many object references #42457

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

Closed
natebosch opened this issue Jun 24, 2020 · 5 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@natebosch
Copy link
Member

natebosch commented Jun 24, 2020

If you have a Future which is retained by GC because it is in the static fields table, it will leak everything in the context of any async closure which awaits it.

For example:

final somethingExpensive = Future.value(1);

void main() async {
  var s = SomethingBig();
  await () async {
    await somethingExpensive;
    s.doSomething();
  }();

  print('All done with SomethingBig');
}

class SomethingBig {
  void doSomething() {}
}

By the time main is complete I hope for SomethignBIg to no longer be retained, however it is still reachable:

SomethingBig {  ⊞  }
retained by offset 3 of Context (1) {  ⊞  }
retained by offset 2 of Context (9) {  ⊞  }
retained by _context@0150898 of Closure (async_op) {  ⊞  }
retained by _awaiter@4048458 of _Future {  ⊞  }
retained by a GC root (static fields table)

After running the following, SomethingBig is able to be garbage collected, because the _valueOrListeners field stops holding the listener after it no longer needs it.

final somethingExpensive = Future.value(1);

void main() async {
  var s = SomethingBig();
  await somethingExpensive.whenComplete(s.doSomething);

  print('All done with SomethingBig');
}

class SomethingBig {
  void doSomething() {}
}
@natebosch natebosch added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jun 24, 2020
@ds84182
Copy link
Contributor

ds84182 commented Oct 5, 2020

Has there been any work in this area? This can cause accidental leaks of native resources in Flutter applications, and there doesn't seem to be much I can do to mitigate these issues.

@mkustermann
Copy link
Member

mkustermann commented Oct 6, 2020

The fact that closure contexts are capturing more than necessary as well as not null-ing out references in the context once it's not needed anymore is a known issue. See also #36983

Fixing the general issue is probably a bigger undertaking, so it needs to be high up on the priority list for us to invest in changing this.

Fixing it might also reduce usability to a certain extend. Because the debugger in JIT would - when hitting a breakpoint in a function - suddently show variables as <optimized-out> even though the variables are in-scope.

Maybe we can mark this as a duplicate of #36983 ?

@ds84182
Copy link
Contributor

ds84182 commented Oct 9, 2020

In some cases the closure context can have a direct reference to something large, so the Closure stashed by the _awaiter field can still retain large objects. Right now it seems that nothing nulls out this field after Future completion.

Context, maybe its worth optionally allowing JIT mode to opt into more restrictive Contexts via a flag.

@mkustermann
Copy link
Member

The particular Future._awaiter we actually can get rid of I believe. We have now added a more general way in the VM to obtain the awaiter of a future via the _resultOrListeners field.

I think it makes sense to abstract this logic out of runtime/vm/stack_trace.cc and make it usable in runtime/vm/debugger.cc and completely remove the Future._awaiter field (same thing for streams as well).

@cskau-g Can you look into this?

@mkustermann mkustermann assigned ghost Oct 9, 2020
dart-bot pushed a commit that referenced this issue Oct 21, 2020
Changes debugger.cc to use the 'lazy async stack' mechanism for
unwinding async stacks, which means we no longer have to manually
keep track of the caller.

This also allows us to get rid of _asyncStarListenHelper.

Bug: #42457
Change-Id: I536e9da4af275998140ae5f05e3a43e07c77cfc7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/167561
Commit-Queue: Clement Skau <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
@ghost
Copy link

ghost commented Oct 23, 2020

I think it makes sense to abstract this logic out of runtime/vm/stack_trace.cc and make it usable in runtime/vm/debugger.cc and completely remove the Future._awaiter field (same thing for streams as well).

This is done with the above change.

I'm not sure if that's the extent of what we wanted to track in the issue or not, but I'll go ahead and close it since the mentioned part is resolve.
If there is in fact more to track then please feel free to reopen.

This issue was closed.
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, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

3 participants