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

Memory grows with each request, PinnedBlockMemoryPool never release memory #55490

Open
1 task done
depler opened this issue May 2, 2024 · 15 comments
Open
1 task done
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@depler
Copy link

depler commented May 2, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Here is code sample:

[Route("api/[controller]")]
[ApiController]
public class TestsController : ControllerBase
{
    static readonly byte[] Data = new byte[1_000_000_000]; //1gb

    [HttpGet(nameof(Download))]
    public ActionResult<byte[]> Download()
    {
        GC.Collect();

        var memory = System.Diagnostics.Process.GetCurrentProcess().PrivateMemorySize64;
        Console.WriteLine($"{memory / 1_000_000} MB");

        return Data;
    }
}

I am calling this method via curl as following 10 times:

curl --insecure https://localhost:5000/api/Tests/Download > NUL

And here is output of the sample:

67 MB
5216 MB
6626 MB
8014 MB
9400 MB
9464 MB
12195 MB
13592 MB
14975 MB
16356 MB

So memory grows over and over, despite the fact that Data is static variable and GC.Collect() called each time. Please someone explain what is going on? And how to reduce memory consumption?

Expected Behavior

Much less memory consumption

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

8.0.204

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label May 2, 2024
@martincostello
Copy link
Member

Can you still repro with this change? Process implements IDisposable and you aren't disposing of it so there's a possibility you're just observing native resources associated with that process handle leaking.

- var memory = System.Diagnostics.Process.GetCurrentProcess().PrivateMemorySize64;
+ long memory;
+ using (var process = System.Diagnostics.Process.GetCurrentProcess())
+ {
+     memory = process.PrivateMemorySize64;
+ }

@teoadal
Copy link

teoadal commented May 3, 2024

I've running it in docker on mcr.microsoft.com/dotnet/aspnet:8.0. Looks like ok, I can't reproduce it.

Processor count: 2
Available memory: 6217 MB

68 MB
2102 MB
3136 MB
3138 MB
3123 MB
3104 MB
3127 MB

Code:

using System.Diagnostics;
using System.Runtime.CompilerServices;

var data = new byte[1_000_000_000];

var app = WebApplication
    .CreateSlimBuilder(args)
    .Build();

Console.WriteLine($"Processor count: {Environment.ProcessorCount}");
Console.WriteLine($"Available memory: {GetAvailableMemory()}");

var downloadApi = app.MapGroup("/download");
downloadApi.MapGet("/", () =>
{
    GC.Collect();
    Console.WriteLine(GetCurrentMemory());
    return Results.File(data, "application/pdf", "test.pdf");
});

await app.RunAsync();

return;

static string GetAvailableMemory()
{
    var memoryInfo = GC.GetGCMemoryInfo();
    return $"{memoryInfo.TotalAvailableMemoryBytes / 1_000_000} MB";
}

[MethodImpl(MethodImplOptions.NoInlining)]
static string GetCurrentMemory()
{
    using var process = Process.GetCurrentProcess();
    return $"{process.WorkingSet64 / 1_000_000} MB";
}

So, I think:

  1. You can run your example in a container to eliminate side effects.
  2. You can read about server garbage collection to understand why memory traffic depends on CPU and available memory.
  3. Make sure that you download only one file in your test at a time. If you will download in parallel the situation will change. You can use ConcurrencyLimiter in your application's configuration to make this rule clear.

@depler
Copy link
Author

depler commented May 3, 2024

Can you still repro with this change? Process implements IDisposable and you aren't disposing of it so there's a possibility you're just observing native resources associated with that process handle leaking.

- var memory = System.Diagnostics.Process.GetCurrentProcess().PrivateMemorySize64;
+ long memory;
+ using (var process = System.Diagnostics.Process.GetCurrentProcess())
+ {
+     memory = process.PrivateMemorySize64;
+ }

Yes, I still can reproduce it. There is probably some leak because of missing Dispose, but not gigabytes.

@depler
Copy link
Author

depler commented May 3, 2024

I've running it in docker on mcr.microsoft.com/dotnet/aspnet:8.0. Looks like ok, I can't reproduce it.

Processor count: 2
Available memory: 6217 MB

68 MB
2102 MB
3136 MB
3138 MB
3123 MB
3104 MB
3127 MB

Code:

using System.Diagnostics;
using System.Runtime.CompilerServices;

var data = new byte[1_000_000_000];

var app = WebApplication
    .CreateSlimBuilder(args)
    .Build();

Console.WriteLine($"Processor count: {Environment.ProcessorCount}");
Console.WriteLine($"Available memory: {GetAvailableMemory()}");

var downloadApi = app.MapGroup("/download");
downloadApi.MapGet("/", () =>
{
    GC.Collect();
    Console.WriteLine(GetCurrentMemory());
    return Results.File(data, "application/pdf", "test.pdf");
});

await app.RunAsync();

return;

static string GetAvailableMemory()
{
    var memoryInfo = GC.GetGCMemoryInfo();
    return $"{memoryInfo.TotalAvailableMemoryBytes / 1_000_000} MB";
}

[MethodImpl(MethodImplOptions.NoInlining)]
static string GetCurrentMemory()
{
    using var process = Process.GetCurrentProcess();
    return $"{process.WorkingSet64 / 1_000_000} MB";
}

So, I think:

  1. You can run your example in a container to eliminate side effects.
  2. You can read about server garbage collection to understand why memory traffic depends on CPU and available memory.
  3. Make sure that you download only one file in your test at a time. If you will download in parallel the situation will change. You can use ConcurrencyLimiter in your application's configuration to make this rule clear.

return Results.File(data, "application/pdf", "test.pdf"); grows up to half of availiable memory.

If you replace it with return data - it will grow up to full memory. Different GC settings does not change the behaviour.

@martincostello
Copy link
Member

There's a number of issues open in dotnet/runtime related to increased memory consumption with containers in .NET 8:

Do any of those correlate with what you're seeing in terms of the runtime environment you use and the work your application typically does?

@depler
Copy link
Author

depler commented May 3, 2024

So, memory is growing up dramatically because of this:

private readonly ConcurrentQueue<MemoryPoolBlock> _blocks = new ConcurrentQueue<MemoryPoolBlock>();

And this: https://github.com/dotnet/aspnetcore/blob/v8.0.4/src/Shared/Buffers.MemoryPool/MemoryPoolBlock.cs

You see? Starting from some response length (megabytes and more) Kestrel uses memory pool to store this response. The problem is that PinnedBlockMemoryPool never release memory - it can only grows up. As far as I know there is no way to disable this behavior, and Microsoft has been promising for 3 years to fix this 🙁

@depler depler changed the title Process memory grows with each request Memory grows with each request, PinnedBlockMemoryPool never release memory May 3, 2024
@depler
Copy link
Author

depler commented May 4, 2024

I've made code sample to demonstrate the problem and way to reduce memory consumption: https://github.com/depler/DotnetMemoryTest

It uses Harmony (https://github.com/pardeike/Harmony) to override original logic of PinnedBlockMemoryPool. See method MemoryPatch.Install();

Test steps are following:

  1. Run DotnetMemoryTest, wait until Tracking memory text in console
  2. Run download_loop.bat file for about 1 minute, then kill it
  3. Wait for about 1 more minute to let GC work and stabilize memory amount

Memory consumption without patch:
image

Memory consumption with patch:
image

So this problem is definitely related with incorrect implementation of PinnedBlockMemoryPool. It is not memory leak by nature, but it leads to accumulation huge amount of almost-not-used byte arrays forever, while process alive. At the very bad scenario - it will lead to OutOfMemoryException during heavy load.

@davidfowl
Copy link
Member

While this is a problem, the easy way to avoid this is to write to the response in chunks instead of allocating large buffers per request. Though the pinned block memory pool has issues, allocating large buffers on the large object heap per request will result in similar issues.

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/best-practices?view=aspnetcore-8.0#minimize-large-object-allocations

@donnyv
Copy link

donnyv commented Nov 5, 2024

I'm seeing this issue when sending small byte array protobuffers.

Its a very straight forward grab blob from sqlite and send it to controller.

using (var conn = new SQLiteConnection($"Data Source={sqlitefile};Version=3;Pooling=True;Read Only=True;FailIfMissing=True;"))
{
    await conn.OpenAsync();

    var sql = $"SELECT tile_data FROM tiles WHERE zoom_level = {zoom} AND tile_column = {x} AND tile_row = {y}";
    using (var cmd = new SQLiteCommand(sql, conn))
    using(var rdr = await cmd.ExecuteReaderAsync())
    {

        if (await rdr.ReadAsync())
        {
            return (false, PlatformConstants.strSuccess, (byte[])rdr[0]);
        }
        else
            return (false, "Not found", Array.Empty<byte>()); 
    }
}

Controller

[HttpGet]
[Route("v1.0/osm/{zoom}/{x}/{y}.pbf")]
public async Task<ActionResult> OsmTile([FromRoute] int zoom, [FromRoute] int x, [FromRoute] int y)
{
    var tile_result = await OsmTilesBL.GetTile(zoom, x, y);

    this.Response.Headers.Add("Content-Encoding", "gzip");

    return File(tile_result.Tile, MimeTypes.application_xprotobuf);
}

@mustafacagataytulun
Copy link

In a Kubernetes environment, this behavior also disrupts the functionality of the Horizontal Pod Autoscaler. When the load increases, the memory used by the pods rises, prompting Kubernetes to spin up new containers. These new containers also consume more memory. However, when the load decreases, the memory usage of the additional pods does not decrease accordingly, leaving the system in a state with, for example, 20 pods, even if there hasn’t been a single request in the past hour.

@davidfowl
Copy link
Member

We're looking into addressing this in .NET 10, dont' expect magic though, the examples in this thread will still perform poorly. The solution is often to stream data and avoiding large, reallocated buffers especially on a per request basis.

@mustafacagataytulun
Copy link

Thanks for the fast response @davidfowl!

We use gRPC in our ASP.NET Core applications. I think the Protobuf serialization is non-streaming (unlike System.Text.Json's JSON serialization. This is mentioned in #27394).

Our scenario includes an Anti-Corruption Layer application which takes gRPC requests from another app (ours), makes the necessary request to a SOAP service (3rd party), receives the SOAP response, fill the gRPC response message, and return it to the caller.

Converting our gRPC services into server-streaming from unary may be a solution for our use case. However, it may introduce unnecessary complexity to both our application and the caller application. A simple depiction of the scenario is below.

gRPC client app ------------> gRPC server app (ACL, this one has the memory issues) ------------> SOAP web service (3rd party)

@davidfowl
Copy link
Member

Streaming might not mean a converting to gRPC streams, it might mean breaking up the big payload into smaller chunks. If you are sending large data payloads over GRPC then it might make sense to use http directly instead (multipart file uploads or streams directly). It all depends on the scenario though. gRPC is fairly optimized with how it writes to the underlying response, though the lack of async in the protobuf serializers makes it not ideal for large payloads.

@mustafacagataytulun where are the memory issues in your pipeline?

@mustafacagataytulun
Copy link

It is the same as the OP, the MemoryPoolBlock. They are not disposed of after a load (a temporary peak in requests to our service). Here is a PerfView image from a GC Dump.

Image

Also, the Flame Graph:

Image

@davidfowl
Copy link
Member

It is the same as the OP, the MemoryPoolBlock. They are not disposed of after a load (a temporary peak in requests to our service). Here is a PerfView image from a GC Dump.

I believe it but the OP returned a 1 GB buffer from each HTTP request, something that I wouldn't recommend anyone do. It might be possible in your scenario to write better code to avoid the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

6 participants