Fix memory leak when serving static content #2880
Conversation
|
|
| { | ||
| context.Response = staticContentResponse; | ||
| tcs.SetResult(context); | ||
| return tcs.Task; |
There was a problem hiding this comment.
Does the TaskCompletionSource use or reference the Cancellation Token? I don't quite understand how the
try ... finally { cts.Dispose() } is any different than
cts.Dispose();
return tsc.Task;
This isn't dependent on the asynchronous execution of anything, unlike the lifeCycleTask below, is it?
There was a problem hiding this comment.
The only difference is that try{...}finally { cts.Dispose();} will still dispose of the CancellationTokenSource whether an exception occurs on the intervening lines or not.
There was a problem hiding this comment.
Ah, going for the equivalent behavior of declaring a using block inside this scope, and there is some possibility of an exception setting the result or the response. That makes more sense to me.
There was a problem hiding this comment.
In theory each of the individual try...finally... could be replaced with separate using (cts) {...} right? (Just working out some personal edification here)
There was a problem hiding this comment.
In these cases, yes, I think it would be functionally the same. Personally, I like being more explicit that we're only disposing of the item here since the typical use of using also instantiates the object and could potentially be misinterpreted by others looking at the code.
Prerequisites
Description
Added calls to dispose the
CancellationTokenSourcewhen the engine returns static content and on errors. Potentially fixes the memory leaks described in #2605 without reverting the fix in #2150