Memory leak in UdpClient on Unix

[stalexig]

Description

I'm using UdpClient to read data asynchronously over Udp. To cancel the operation after a timeout, I use CancellationTokenSource. I noticed that with a slow device, the timeout often triggers and the memory begins to grow. DotMemory shows a constant increase in the number of these objects:

Reproduction Steps

The problem only exists on Unix systems, there is no leak on Windows.

Here is test code that reproduces the described problem:

using var udpClient = new UdpClient();
udpClient.Connect(IPAddress.Parse(addr), port);

while (true)
{
    try
    {
        using var tokenSource = new CancellationTokenSource(100);
        var result = await udpClient.ReceiveAsync(tokenSource.Token);
    }
    catch (OperationCanceledException)
    {
    }
    
    await Task.Delay(100).ConfigureAwait(false);
}

Expected behavior

All resources should be released after ReceiveAsync exits with a cancellation token

Actual behavior

There is a memory leak on Unix

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[wfurt]

we should figure out if they are rooted (e.g. leak) or if that is just lazy GC. Do you see it grow if you add GC.Collect to the loop @stalexig
UDPClient is just very thin layer on top of Socket and I did not see any suspicious code there (like pool allocations)

[stalexig]

1. Adding GC.Collect to the test project does not solve the problem - the memory continues to grow.

2. I also tried using sockets directly without UdpClient. The code below has the same problem:

var inBuffer = new byte[64 * 1024];
    
using var socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
var ipEndPoint = new IPEndPoint(IPAddress.Any, 0);
socket.Bind(ipEndPoint);

while (true)
{
    try
    {
        using var tokenSource = new CancellationTokenSource(100);
        
        var remote = new IPEndPoint(IPAddress.Any,0);
        var result = await socket
            .ReceiveFromAsync(inBuffer, SocketFlags.None, remote, tokenSource.Token)
            .ConfigureAwait(false);
    }
    catch (OperationCanceledException)
    {
        
    }
    
    GC.Collect();
    
    await Task.Delay(100).ConfigureAwait(false);
}

3. I'm using dotmemory for leak analysis. In this utility, in the Retention tab, I see a long linked list of "System.Net.Sockets.SocketAsyncContext+BufferMemoryReceiveOperation" objects:

The number of objects in this list is constantly growing, although the code shows that I am not performing more than one read operation at a time. The Retention tab also shows that these objects has references to the CancelationToken and byte[] buffer. The comparison of snapshots shows that their number is also continuously growing:

I looked at the Socket code for Unix and I have a feeling that when the cancellation token fires, the read operation is not removed from the linked list of operations in the SocketAsyncContext.

There may be a bug in my code, but it is very simple and works without problems on Windows. I don't have any ideas yet what could be wrong.

[NinoFloris]

Might be related to this comment:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Lines 237 to 238 in 6afd223

	// Note, we leave the operation in the OperationQueue.
	// When we get around to processing it, we'll see it's cancelled and skip it.

[antonfirsov]

Might be related to this comment:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Lines 237 to 238 in 6afd223

	// Note, we leave the operation in the OperationQueue.
	// When we get around to processing it, we'll see it's cancelled and skip it.

This is true, we leave all cancelled operations in the queue. While for TCP this doesn't lead to issues since a TCP socket is considered to be unusable after a cancellation, with UDP, continuing working with the socket might be valid. Regardless, I'm not sure if the impact of this issue is high.

with a slow device, the timeout often triggers and the memory begins to grow

@stalexig does this happen with short timeouts? What is the goal of the timeout if you will retry the recieve anyways?

[stalexig]

@antonfirsov
I can describe my situation: in my case, the program requests data from hundreds of devices. The timeout is needed because the device may simply not respond to the request if it is too busy. So there is no point in waiting too long, the request will just need to be repeated later.

However, the device may simply be turned off for repair. Or a whole group of devices may be turned off. In this case, the requests also go away, but there is no response, and the read operation is cancelled due to a timeout. The memory immediately begins to grow. To avoid this I have to recreate the socket every time I cancel reading operation.

[wfurt]

When would we cleanup the queue @antonfirsov? When the socket is closed or is that truly a leak? Or would we re-use the operation on next read?

[antonfirsov]

When would we cleanup the queue

When a socket is closed, SocketAsyncContext is abandoned together with its queues, so this isn't truly a leak. However as #111400 (comment) notes, the current behavior forces the user to throw away & recreate the socket after cancellation. This wouldn't be necessary if we removed the cancelled operation from the queue when cancellation happens. We do this for sync-over async timeouts:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Lines 1424 to 1428 in d34ef7e

	if (timeoutExpired)
	{
	queue.CancelAndContinueProcessing(operation);
	operation.ErrorCode = SocketError.TimedOut;
	}

I don't know why we decided to leave the operation in the queue for async cancellations.