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

Change disposal of CancellationTokenSource to after task is complete #2150

Merged

Conversation

jeffthiele
Copy link
Contributor

The previous implementation was allowing the disposal of the CancellationTokenSource to occur before the lifeCycleTask was actually complete. I moved the disposal to the completeTask continuation so it would only be disposed when the task was complete.

Also included is a test that can detect this in the future and also verifies that the token is disposed after the call.

I also ran through the memory leak test in pull request #2128 and (assuming I had my CLI commands correct for dotMemoryUnit) it passed.

@jeffthiele
Copy link
Contributor Author

I forgot to mention, this is to fix issue #2141

}
finally
{
cts.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

damianh added a commit that referenced this pull request Dec 7, 2015
…eDisposal

Change disposal of CancellationTokenSource to after task is complete
@damianh damianh merged commit dc5e050 into NancyFx:master Dec 7, 2015
@khellang khellang added this to the 1.4.3 milestone Dec 8, 2015
@khellang khellang added the Bug label Dec 8, 2015
@jeffthiele jeffthiele deleted the FixCancellationTokenSourceDisposal branch December 8, 2015 14:51
@damianh
Copy link
Member

damianh commented Dec 10, 2015

There is a PR where the async await stuff is cleaned up for .net 4.5 : https://github.com/NancyFx/Nancy/pull/2105/files#diff-3711e290f8f06e8bd6ce86e1e6ce7dbfR102

I don't think I need to do any special here?

@jeffthiele
Copy link
Contributor Author

The async/await implementation looks correct to me too

@damianh
Copy link
Member

damianh commented Dec 10, 2015

Thanks @jeffthiele-iq

@thecodejunkie
Copy link
Member

@jeffthiele-iq appreciate your feedback on async/await ! ❤️

@khellang
Copy link
Member

Version 1.4.3 was just pushed to NuGet.

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.

4 participants