AsyncIO may have a memory leak problem

[GuojieLin]

I've been using NetMQ in production for a while. NETMQ version number is 4.0.0.1,AsyncIO version number is 0.1.26.0
Recently, i found the program had crashed. Checked out the windows log to find out is an OOM exception。
Then I view the dump file through windbg.

!dumpheap -stat 
...
00007ff7ffbc0d50   536240     17159680 NetMQ.Core.Utils.Proactor+Item
00007ff7ffbca7f8   536242     17159744 NetMQ.Core.IOObject
00007ff7ffbcba70   536534     34338176 AsyncIO.Windows.AcceptExDelegate
00007ff7ffbcb7f0   536534     34338176 AsyncIO.Windows.ConnectExDelegate
00007ff7ffbcbdd8  1073068     60091808 AsyncIO.Windows.Overlapped
00007ff7ffbcb600   536534     90137712 AsyncIO.Windows.Socket
Total 3839215 objects

I checked the status of several objects. Confirm that the AsyncSocket object has called the Dispose method，and the value of InProgress equels true

!mdt 000000000390db20
000000000390db20 (AsyncIO.Windows.Overlapped)
    m_address:000000001ef5ffb0 (System.IntPtr)
    m_handle:(System.Runtime.InteropServices.GCHandle) VALTYPE (MT=00007ff85e5f3bc0, ADDR=000000000390db48)
    <OperationType>k__BackingField:0x1 (Receive) (AsyncIO.OperationType)
    <AsyncSocket>k__BackingField:000000000390da78 (AsyncIO.Windows.Socket)
    <InProgress>k__BackingField:true (System.Boolean)
    <Disposed>k__BackingField:true (System.Boolean)
    <State>k__BackingField:000000000390de08 (NetMQ.Core.Utils.Proactor+Item)

check gc table

!gchandles
GC Handle Statistics:
Strong Handles: 520519
Pinned Handles: 84
Async Pinned Handles: 0
Ref Count Handles: 0
Weak Long Handles: 43
Weak Short Handles: 116
Other Handles: 0
Statistics:
...
00007ff7ffbcbdd8   511752     28658112 AsyncIO.Windows.Overlapped
Total 520762 objects

check Finalize Queue

!finq -stat
Generation 0:
       Count      Total Size   Type
---------------------------------------------------------
           1             168   AsyncIO.Windows.Socket

1 object, 168 bytes

Generation 1:
       Count      Total Size   Type
---------------------------------------------------------
        1008          169344   AsyncIO.Windows.Socket
           2              48   System.Windows.Forms.VisualStyles.VisualStyleRenderer+ThemeHandle

1,010 objects, 169,392 bytes

Generation 2:
       Count      Total Size   Type
---------------------------------------------------------
           1             776   FC.Main.frmMain
           1             104   AsyncIO.Windows.CompletionPort
      535525        89968200   AsyncIO.Windows.Socket
...

It can be concluded that AsyncSocket called Dispose, but because the overlapped operation was being processed, it was not eventually notified of the io completion port

Then I found a lot of junk connection requests in the program log. I found through wireshark clutch bag due to multiple abnormal connections

i check the source code

public override void Receive(byte[] buffer, int offset, int count, SocketFlags flags)
{
m_inOverlapped.StartOperation(OperationType.Receive);

            m_receiveWSABuffer.Pointer = new IntPtr(m_receivePinnedBuffer.Address + offset);
            m_receiveWSABuffer.Length = count;

            int bytesTransferred;

            SocketError socketError = UnsafeMethods.WSARecv(Handle, ref m_receiveWSABuffer, 1,
              out bytesTransferred, ref flags, m_inOverlapped.Address, IntPtr.Zero);

            if (socketError != SocketError.Success)
            {
                socketError = (SocketError)Marshal.GetLastWin32Error();

                if (socketError != SocketError.IOPending)
                {
                    throw new SocketException((int)socketError);
                }
            }   
}

if socket was reset when need to read and write ,the asyncsocket will be dispose and the Io operation canceled. InProgress equels true so that the Overlapped hand not be free. it may cause io completion notification may never come.

Is this and 《Could it be a potential issue of AsyncSocket? #26》is same problem?

[somdoron]

Thanks. I will take a look.
If you have a fix in mind that will be great

[wmjordan]

I tracked the situation during my development with AsyncIO.

If Overlapped is in progress while AsyncSocket is being disposed, I put them into a "trash bin" (a static collection) and another worker thread will poll that "bin" and recycle items within after a pretty decent time.

[wmjordan]

@GuojieLin
The problem you reported seemed to have nothing to do with #26.

Nevertheless, your problem is valid.
You need to modify the code in Overlapped.cs to fix this problem.

The "trash bin" solution aforementioned, I think, is dirty, since it requires another thread to clean up things and the appropriate timing is hard to predict, hence I did not make a PR.

[GuojieLin]

@wmjordan @somdoron
Thanks for the quick reply.
In fact, I tested the situation under which the client connection was reset immediately after the connection was established.
Call UnsafeMethods.WSARecv return SocketError.ConnectionReset ,and it will throw SocketException。The upper code will releases the socket. that's probably the case.
i try do this

public override void Receive(byte[] buffer, int offset, int count, SocketFlags flags)
{
       ...
            int bytesTransferred;

            SocketError socketError = UnsafeMethods.WSARecv(Handle, ref m_receiveWSABuffer, 1,
              out bytesTransferred, ref flags, m_inOverlapped.Address, IntPtr.Zero);

            if (socketError != SocketError.Success)
            {
                socketError = (SocketError)Marshal.GetLastWin32Error();

                if (socketError != SocketError.IOPending)
                {
                    throw new SocketException((int)socketError);
                }
            }
            m_inOverlapped.StartOperation(OperationType.Receive);
}

the Send Method is same
i found the memory leak is gone.
if the method call return is not SocketError.Success or SocketError.IOPending i think the Overlapped is not processing. it should be released safely. do you think so?
Does it cause anything else?

[wmjordan]

I noticed that there was a m_receiveWSABuffer in your code snippet, which did not exist in the source code of AsyncIO. How did you define and access that member?

[GuojieLin]

@wmjordan the asyncio version is 0.1.26.0 , I merged all previous submited code from November 4, 2017 to my local branch.

public override void Receive(byte[] buffer, int offset, int count, SocketFlags flags)
        {
            if (buffer == null)
                throw new ArgumentNullException("buffer");

            if (m_receivePinnedBuffer == null)
            {
                m_receivePinnedBuffer = new PinnedBuffer(buffer);
            }
            else if (m_receivePinnedBuffer.Buffer != buffer)
            {
                m_receivePinnedBuffer.Switch(buffer);
            }

            m_inOverlapped.StartOperation(OperationType.Receive);

            m_receiveWSABuffer.Pointer = new IntPtr(m_receivePinnedBuffer.Address + offset);
            m_receiveWSABuffer.Length = count;

            int bytesTransferred;

            SocketError socketError = UnsafeMethods.WSARecv(Handle, ref m_receiveWSABuffer, 1,
              out bytesTransferred, ref flags, m_inOverlapped.Address, IntPtr.Zero);

            if (socketError != SocketError.Success)
            {
                socketError = (SocketError)Marshal.GetLastWin32Error();

                if (socketError != SocketError.IOPending)
                {
                    throw new SocketException((int)socketError);
                }
            }
        }

[wmjordan]

Oh, I see.
It was the earlier code of AsyncIO. Afterward I refactored the PinnedBuffer into Overlapped and substituted the m_receiveWSABuffer member by a local variable.

@somdoron I think we should further refactor the Overlapped and Windows.Socket class.
The original purpose of Overlapped.StartOperation is to mark the operation type, reset the success marker, denote that an overlapped operation is in progress.
As @GuojieLin 's modification, the StartOperation was moved to the end of the call. That could be problematic in multi-threading environment, since WSARecv is not guarded from being called concurrently.

Instead, I suggest:

1. making Overlapped.InProgress a volatile field and use Interlocked.CompareExchange to modify it in order to block concurrent overlapped operation on the same Overlapped instance;
2. reset that field and release the pinned buffer when WSARecv returns a SocketError other than Success or IOPending. (will this be really safe?)

[GuojieLin]

@wmjordan
you are right. it should be marked as volatile.
I looked up the information about overlapped.
《windows via c/c++ 5nd》-10.4.2 about Precautions for asynchronous device I/O:

1. If GetLastError returns a value equeal ERROR_IO_PENDING ,then the I/O request has been successfully enqueue and will be completed later.
2. If GetLastError returns a value other than ERROR_IO_PENDING. I/O requests cannot be added to the queue for device drivers.

[GuojieLin]

If no error occurs and the send operation has completed immediately, WSASend returns zero. In this case, the completion routine will have already been scheduled to be called once the calling thread is in the alertable state. Otherwise, a value of SOCKET_ERROR is returned, and a specific error code can be retrieved by calling WSAGetLastError. The error code WSA_IO_PENDING indicates that the overlapped operation has been successfully initiated and that completion will be indicated at a later time. Any other error code indicates that the overlapped operation was not successfully initiated and no completion indication will occur.

WSASend function