Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Memory Leak in Async code #1506

Closed
WayneHiller opened this issue Sep 29, 2020 · 11 comments
Closed

Possible Memory Leak in Async code #1506

WayneHiller opened this issue Sep 29, 2020 · 11 comments

Comments

@WayneHiller
Copy link

WayneHiller commented Sep 29, 2020

I believe I have discovered a memory leak in Http.Async.cs

void SetTimeout(IAsyncResult asyncResult)
        {
            if (Timeout != 0)
                ThreadPool.RegisterWaitForSingleObject(
                    asyncResult.AsyncWaitHandle,
                    TimeoutCallback, _timeoutState, Timeout, true
                );

            static void TimeoutCallback(object state, bool timedOut)
            {
                if (!timedOut)
                    return;

                if (!(state is TimeOutState tos))
                    return;

                lock (tos) tos.TimedOut = true;

                tos.Request?.Abort();
            }
        }

The handle In the call to ThreadPool.RegisterWaitForSingleObject is not saved and handle.Unregister is never called.

https://docs.microsoft.com/en-us/dotnet/api/system.threading.threadpool.registerwaitforsingleobject?view=netcore-3.1

My ANTS Memory Profiler is showing the leak.

image

Specifications

  • Version: 106.10.1
  • Platform: Win x64
@WayneHiller
Copy link
Author

WayneHiller commented Sep 29, 2020

This could be a fix:

class TimeOutState
{
      public bool TimedOut { get; set; }
      public HttpWebRequest? Request { get; set; }
      public RegisteredWaitHandle Handle = null;
}

...

void SetTimeout(IAsyncResult asyncResult)
        {
            if (Timeout != 0)
                _timeoutState.Handle = ThreadPool.RegisterWaitForSingleObject(
                    asyncResult.AsyncWaitHandle,
                    TimeoutCallback, _timeoutState, Timeout, true
                );

            static void TimeoutCallback(object state, bool timedOut)
            {
                if (!(state is TimeOutState tos))
                    return;

                lock(tos)
                {
                    if(!timedOut)
                    {
                        if(tos.Handle != null)
                        {
                            tos.Handle.Unregister(null);
                        }
                        return;
                    }

                    tos.TimedOut = true;
                }

                tos.Request?.Abort();
            }

@WayneHiller
Copy link
Author

WayneHiller commented Sep 29, 2020

I cloned the repo, made the above changes and tested it in my project. The leak was fixed. Any idea why the Repo I cloned is version 106.8.10 but the nuget version I have already in the project is 106.10.1?

@mhornbacher
Copy link
Contributor

mhornbacher commented Dec 3, 2020

Is there a Pull Request for this fix?

@WayneHiller
Copy link
Author

WayneHiller commented Dec 3, 2020

I did not create a Pull Request because the version number of the Repo I cloned was older than the release.

@mhornbacher
Copy link
Contributor

mhornbacher commented Dec 3, 2020

@WayneHiller mind if I take a crack at it? Its effecting our production code.

@WayneHiller
Copy link
Author

WayneHiller commented Dec 3, 2020

@mhornbacher Go for it.

alexeyzimarev pushed a commit that referenced this issue Dec 9, 2020
@alexeyzimarev
Copy link
Member

alexeyzimarev commented Dec 9, 2020

Ok, it's merged, @WayneHiller would you be able to check it against your test with the latest alpha nuget package?

@WayneHiller
Copy link
Author

WayneHiller commented Dec 9, 2020

I have since changed the code to use HttpClient, I won't be able to test it. I did test the code above and the leak was fixed.

@mhornbacher
Copy link
Contributor

mhornbacher commented Dec 15, 2020

@alexeyzimarev we deployed this beta package to our QA environment and validated that the memory leak is indeed fixed.

@alexeyzimarev
Copy link
Member

alexeyzimarev commented Dec 15, 2020

@mhornbacher awesome, I will make a fix release soon for the stable package.

@mhornbacher
Copy link
Contributor

mhornbacher commented Dec 15, 2020

Thank you!

Should we close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants