Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Fix memory leak when serving static content #2880

Open
wants to merge 1 commit into
base: 1.x-WorkingBranch
Choose a base branch
from
Open

Fix memory leak when serving static content #2880

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/Nancy/NancyEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,16 @@ public Task<NancyContext> HandleRequest(Request request, Func<NancyContext, Nanc
var staticContentResponse = this.staticContentProvider.GetContent(context);
if (staticContentResponse != null)
{
context.Response = staticContentResponse;
tcs.SetResult(context);
return tcs.Task;
try
{
context.Response = staticContentResponse;
tcs.SetResult(context);
return tcs.Task;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory each of the individual try...finally... could be replaced with separate using (cts) {...} right? (Just working out some personal edification here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
finally
{
cts.Dispose();
}
}

var pipelines = this.RequestPipelinesFactory.Invoke(context);
Expand Down Expand Up @@ -147,7 +154,14 @@ public Task<NancyContext> HandleRequest(Request request, Func<NancyContext, Nanc
},
errorTask =>
{
tcs.SetException(errorTask.Exception);
try
{
tcs.SetException(errorTask.Exception);
}
finally
{
cts.Dispose();
}
},
true);

Expand Down