microsoft / ApplicationInsights-SDK-Labs

Application Insights experimental projects repository
MIT License
61 stars 48 forks source link

Fix memory leak when using client dependency telemetry tracking feature #121

Closed AlexFera closed 7 years ago

AlexFera commented 7 years ago

Summary: ClientTelemetryDuplexChannel instances are not removed from the internal Hashtable used by CommunicationObjectManager\ after the channel has been closed due to the incorrect object being passed to the OnClosed event handler.

Our company discovered this issue after we added the Microsoft.ApplicationInsights.Wcf package to our web application so that we can record telemetry between our web application and our internal WCF services exposed over Net.TCP. After a few hours running in Production we discovered a very high memory usage in the web application.

To reproduce this problem you need the following:

I've attached a screenshot from the memory profiler that shows the instances not being released. memory-leak

After further investigation, I discovered that the CommunicationObjectManager\ from the System.ServiceModel.Channels namespace was not removing the ClientTelemetryDuplexChannel instances from the internal Hashtable data structure after the channel was closed. This is due to the fact that the when invoking the Closed event from ClientTelemetryChannelBase the wrong object is being passed to the handler void OnItemClosed(object sender, EventArgs args) of CommunicationObjectManager\, instead of an instance of Microsoft.ApplicationInsights.Wcf.Implementation.ClientTelemetryDuplexChannel an instance was of System.ServiceModel.Channels.ClientFramingDuplexSessionChannel was being sent to the handler, so no item was being removed from the Hashtable.

msftclas commented 7 years ago

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request. Thanks, Microsoft Pull Request Bot

tomasr commented 7 years ago

Very nice catch, Alex! I had been trying to track this one for a while, but could never get the leak to reproduce fully (meaning I was not seeing the substantially high memory usage; probably because I wasn't being able to push it hard enough).

But it does need something else: I think we need to modify OnChannelClosing and OnChannelFaulted in a similar way. Would you include that to the PR?

AlexFera commented 7 years ago

Hi Tomas,

I've included the requested modifications into the PR.

I'm glad that I could help.