Nancy icon indicating copy to clipboard operation
Nancy copied to clipboard

Published 20 hours ago • verified publisher icon NancyFx

Fix a memory leak

Open yzhou88 opened this issue 9 years ago • 10 comments

Prerequisites

  • [x] I have written a descriptive pull-request title
  • [x] I have verified that there are no overlapping pull-requests open
  • [x] I have verified that I am following the Nancy code style guidelines
  • [x] 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.

yzhou88 avatar Oct 30 '16 22:10 yzhou88

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

thecodejunkie avatar Oct 30 '16 22:10 thecodejunkie

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 ?

yzhou88 avatar Oct 30 '16 22:10 yzhou88

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

thecodejunkie avatar Oct 30 '16 22:10 thecodejunkie

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!

yzhou88 avatar Oct 30 '16 22:10 yzhou88

This will just bring back the bug that https://github.com/NancyFx/Nancy/pull/2150 tried to fix. We need to figure out exactly what and when we're leaking memory here...

khellang avatar Oct 31 '16 21:10 khellang

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.

jeffthiele avatar Dec 06 '16 21:12 jeffthiele

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 avatar Dec 30 '16 10:12 khellang

@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 https://github.com/NancyFx/Nancy/issues/2113 a long time ago, it was pretty easy to see exactly what was leaking :smile:

khellang avatar Dec 30 '16 10:12 khellang

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?

cloudhunter89 avatar Mar 19 '18 18:03 cloudhunter89

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

jeffthiele avatar Mar 19 '18 18:03 jeffthiele