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

Fix a memory leak #2605

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

Conversation

yzhou88
Copy link

@yzhou88 yzhou88 commented Oct 30, 2016

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the Nancy code style guidelines
  • I have provided test coverage for my change (where applicable)

Description

Dispose the object returned by CreateLinkedTokenSource(...)

I have spent around 6 hours working on memory leak issue. After applying the change here, no memory leak anymore.

Dispose the object returned by CreateLinkedTokenSource(...)
@thecodejunkie
Copy link
Member

Let's loop in @khellang and this one as he worked on it previously. Thanks

@thecodejunkie thecodejunkie added this to the 2.0-clinteastwood milestone Oct 30, 2016
@yzhou88
Copy link
Author

yzhou88 commented Oct 30, 2016

I understand this issue does not exist in v2.00. But can you update v1.4.3 so Nuget can give me a bug-free 1.x version ?

@thecodejunkie
Copy link
Member

Oh sorry, you're sending this to the 1.x branch. I'll have a look

@thecodejunkie thecodejunkie removed this from the 2.0-clinteastwood milestone Oct 30, 2016
@yzhou88
Copy link
Author

yzhou88 commented Oct 30, 2016

Wow, I really like to contribute to this project, we have such an active people like you!
When stress testing my project, the memory leak became a serious problem. Please understand that this simple fix at least cost me 6 hours. And I'd like to see v1.4.4 just to have this issue fixed.

Thanks!

@khellang
Copy link
Member

khellang commented Oct 31, 2016

This will just bring back the bug that #2150 tried to fix. We need to figure out exactly what and when we're leaking memory here...

@jeffthiele
Copy link
Contributor

Looking at the original fix for #2150 it looks like I may have missed a cts.Dispose(); in the error task for WhenCompleted() potentially causing a memory leak when errors occur. I wonder if that would solve the memory leaks you're seeing.

@khellang
Copy link
Member

I think this is worth fixing for a new 1.4.x release of Nancy, along with #2616. What's the next step here? We certainly won't bring back the behavior of this PR as it currently stands.

@khellang
Copy link
Member

@yzhou88 Are you able to provide a bit more information as to when it's leaking memory? Is it per request, or only in certain scenarios? You can use something like dotMemory to profile and see what's going on. When I profiled #2113 a long time ago, it was pretty easy to see exactly what was leaking 😄

@cloudhunter89
Copy link

Looking at the original fix for #2150 it looks like I may have missed a cts.Dispose(); in the error task for WhenCompleted() potentially causing a memory leak when errors occur.

@jeffthiele-iq What about the case of serving static content? It seems that the CancellationTokenSource cts is not disposed of in that case either, or am I missing something else?

@jeffthiele
Copy link
Contributor

jeffthiele commented Mar 19, 2018

Yeah you're right @cloudhunter89, it isn't disposed correctly for static content. None of my testing used static content so I missed that.

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.

None yet

5 participants