Skip to content

Call AsyncIteratorMethodBuilder.Complete before SetResult#47081

Merged
jcouv merged 1 commit intodotnet:masterfrom
jcouv:async-leak
Oct 17, 2020
Merged

Call AsyncIteratorMethodBuilder.Complete before SetResult#47081
jcouv merged 1 commit intodotnet:masterfrom
jcouv:async-leak

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Aug 23, 2020

While investigating a leak in async-iterator scenario, Stephen pointed out that we should call AsynciteratorMethodBuilder.Complete sooner (before notifying caller of result or exception).
It doesn't looks like that's the cause of the leak, but is still worthwhile to fix.

@jcouv jcouv marked this pull request as ready for review August 25, 2020 01:41
@jcouv jcouv requested a review from a team as a code owner August 25, 2020 01:41
@jcouv
Copy link
Member Author

jcouv commented Sep 15, 2020

FYI @stephentoub. Let me know if any second thoughts on this ordering. Thanks

@jcouv
Copy link
Member Author

jcouv commented Sep 15, 2020

@cston @AlekseyTs @jaredpar for a second review. Thanks

@stephentoub
Copy link
Member

@jcouv, I'm fairly certain the new ordering is the right ordering, and I'm a little surprised it didn't fix the cited issue. Now that I'm back from being out, I'll find time to investigate the cited issue. Feel free to hold off on this until then if you don't mind having the open PR around.

@jcouv jcouv marked this pull request as draft September 16, 2020 02:21
@jcouv
Copy link
Member Author

jcouv commented Sep 16, 2020

@stephentoub Okay. I'll mark the PR as draft for now. Feel free to use this branch (open Roslyn.sln, hit F5) to try the fix.
I was careful in testing impact on leak, so I'm fairly confident despite the variability in repro (I tried a few times without fix and a few times with).

@jcouv
Copy link
Member Author

jcouv commented Oct 14, 2020

@stephentoub Did you get a chance to take a look at the leak? Should I move forward with this change? Let me know if I can help.

@stephentoub
Copy link
Member

Thanks, Julien. Looking at it now.

@stephentoub
Copy link
Member

@jcouv, go ahead with this change. I finally made time with a debugger and figured out the problem. This PR is necessary but not sufficient. Will describe the problem in the dotnet/runtime#41187.

@jcouv jcouv marked this pull request as ready for review October 16, 2020 20:09
@jcouv
Copy link
Member Author

jcouv commented Oct 16, 2020

@cston @AlekseyTs @jaredpar for a second review. Thanks

@jcouv jcouv merged commit c838667 into dotnet:master Oct 17, 2020
@ghost ghost added this to the Next milestone Oct 17, 2020
@jcouv jcouv deleted the async-leak branch October 17, 2020 06:13
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants