Prevent GC of value during async eval#9885
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes garbage collection issues with the result list in asyncEval on both native VM and web by keeping a strong reference to the reader object in the target app until the future resolves. The reviewer pointed out that using Timer.periodic inside the evaluated expression might fail if the target library does not explicitly import dart:async. They suggested using an asynchronous loop with Future.delayed instead, as Future is part of dart:core and guaranteed to be available in any Dart library context.
| ## Debugger updates | ||
|
|
||
| TODO: Remove this section if there are not any updates. | ||
| * Prevent values from being garbage-collected, while being evaluated. |
| ' for (int i = 0; i < 100; i++) {' | ||
| ' final _ = reader;' | ||
| ' await Future.delayed(const Duration(milliseconds: 50));' | ||
| ' /* Stop pinning after a 1-second buffer when the future has completed. */' |
There was a problem hiding this comment.
why are we using /* instead of // ?
There was a problem hiding this comment.
As far as I can tell, this is all one line of code. So we can't use EOL comments.
| ' final _ = reader;' | ||
| ' await Future.delayed(const Duration(milliseconds: 50));' | ||
| ' /* Stop pinning after a 1-second buffer when the future has completed. */' | ||
| ' if (isDone || ++bufferTicks > 20) break;' |
There was a problem hiding this comment.
is the change from 100 to 20 intentional?
There was a problem hiding this comment.
if so, we don't need to loop 100 times since we will always short circuit at 20. We can just change the for loop to a while loop
2 participants
Gemini suggests this to reduce flakiness in integration tests, some of which strongly rely on eval. Gemini looked at recent failed integration tests to suggest this.