bugsnag / bugsnag-dotnet

.NET notifier for BugSnag. Error monitoring and error reporting for .NET.
https://docs.bugsnag.com/platforms/dotnet/
MIT License
60 stars 29 forks source link

Asp.Net Client can leak memory and trigger large number of exceptions #164

Open oldcookie opened 1 year ago

oldcookie commented 1 year ago

Describe the bug

Asp.Net client would keep adding the same middleware to the static _globalClient if Bugsnag.Client.Current is called outside of a request scope and call to BeginNotify() is called subsequently to add a middleware.

This results in a memory leak since _globalClient is static. When Notify() is called, this also results a very long running loop and a lot of handled exceptions due to the large number of middleware linked to the globalclient.

Steps to reproduce

  1. Invoke a async function with ConfigureAwait(false) or use Task.Run()
  2. Get a BugSnag client using Bugsnag.Client.Current when the HttpContext.current is no long in scope.
  3. Add a middleware.
  4. Repeat this across many requests
  5. Trigger Notify() on the global client

You would see a lot of exceptions in performance counters.

Environment

BeforeNotify() should probably check if it is using the _globalClient and not add a middleware or middleware should be stored using a HashSet instead to prevent duplicates.

johnkiely1 commented 1 year ago

Hi @oldcookie, Thanks for raising we are going to look to get this fixed. In the meantime it would be interesting to know if you've managed to work around it?

oldcookie commented 1 year ago

Yes, we added a check for HttpContext.current != null before adding a middleware, which bypasses the situation where this turns into an issue.