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

Properly dispose CancellationTokenSource #2114

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

khellang
Copy link
Member

This fixes a memory leak of 40 bytes per request, introduced in 4aeaf31.

Because of the nesting (in the using block), this is best reviewed with ?w=1 😉

We need to push this as 1.4.2 ASAP.

Closes #2113.

@@ -162,7 +163,7 @@ public Task<NancyContext> HandleRequest(Request request, Func<NancyContext, Nanc
/// </summary>
public virtual void Dispose()
{
this.engineDisposedCts.Cancel();
this.engineDisposedCts.Dispose();
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume Dispose will do The Right Thing™ and cancel before cleaning up...

@khellang
Copy link
Member Author

Before

$ wrk -t12 -c400 -d30s http://10.211.55.3:8888/nancy/
Running 30s test @ http://10.211.55.3:8888/nancy/
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   865.58ms  255.22ms   1.86s    71.11%
    Req/Sec    26.59     18.56   120.00     74.73%
  8237 requests in 30.10s, 10.02MB read
  Socket errors: connect 155, read 0, write 0, timeout 0
Requests/sec:    273.64
Transfer/sec:    340.98KB

image

After

$ wrk -t12 -c400 -d30s http://10.211.55.3:8888/nancy/
Running 30s test @ http://10.211.55.3:8888/nancy/
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   866.75ms  255.64ms   1.99s    71.21%
    Req/Sec    26.96     18.93   110.00     71.71%
  8208 requests in 30.09s, 9.99MB read
  Socket errors: connect 155, read 0, write 0, timeout 0
Requests/sec:    272.78
Transfer/sec:    339.91KB

image

thecodejunkie added a commit that referenced this pull request Nov 16, 2015
Properly dispose CancellationTokenSource
@thecodejunkie thecodejunkie merged commit 066fee8 into NancyFx:master Nov 16, 2015
@khellang khellang deleted the memory-leak branch November 16, 2015 08:53
@damianh
Copy link
Member

damianh commented Nov 16, 2015

👍 @khellang

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nancy 1.4.1 memory leak
3 participants