codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.46k stars 1.09k forks source link

Memory leak occurs when QR codes used Base64QRCode class #466

Closed emanuelrm closed 3 months ago

emanuelrm commented 1 year ago

Type of issue

[X] Bug
[ ] Question (e.g. about handling/usage)
[ ] Request for new feature/improvement

Expected Behavior

To generate QR codes through endpoints, is there any recommended approach or is there a standard way? var qrGenerator = new QRCodeGenerator(); var input = "00263fd622"; var qrCodeData = qrGenerator.CreateQrCode(input, QRCodeGenerator.ECCLevel.Q); var qrCode = new Base64QRCode(qrCodeData); var qrCodeImage = qrCode.GetGraphic(20);

Current Behavior

When there are multiple calls to the endpoint that contains the QR code generation code, the application's memory keeps increasing. While debugging, I was able to verify that this happens when the CreateQrCode method is invoked.

The application starts using 82.7 MB. Screenshot from 2023-07-14 16-31-40

After 20 requests, the memory increases to 398.9MB. Screenshot from 2023-07-14 16-32-15

After 100 requests, the memory increases to 1GB. Screenshot from 2023-07-14 16-33-26

I have already tried calling the dispose methods of the qrGenerator, qrCodeData, and qrCode objects. I have also used the using statements for these objects and directly called GC.Collect(), but none of them were able to deallocate from memory.

Possible Solution (optional)

Steps to Reproduce (for bugs)

QrCoderWebTests.tar.gz I created (POC) with the mentioned scenario, execute dotnet run, call endpoint http://localhost:5098/ and used a memory monitoring tool to track memory usage.

Your Environment

Version used: 1.4.1 Environment : .net Core 6

emanuelrm commented 1 year ago

I performed the same scenario using the PngByteQRCode class and then converting to base 64 and now it had the expected behavior, so the problem may be in the Base64QRCode class Screenshot from 2023-07-17 10-17-06

Shane32 commented 3 months ago

I took a long look at this. I don't think there's an actual bug here. Ultimately I think what's happening is that there is enough memory being allocated such that the majority of memory gets bumped to generation 1/2 where it sits for a long time before being collected. It seems to be a combination effect of (1) ASP.NET Core (2) generating the QR code matrix (3) using System.Drawing.Common to generate the bitmap (4) the long input string. Take away any one part and the effect is greatly minimized. However, regardless of the amount of memory that gets allocated, there's no memory leak. The process simply allocates memory up to a threshold, 1.3GB on my PC, and then it performs more GC generation-2 collections to compensate.

To test this I've downloaded the sample program above, and added this immediately prior to the return:

Debug.WriteLine($"Total memory: {GC.GetTotalMemory(false)}");

Running the program and repeatedly downloading the image then produces results like the following:

...

Total memory: 626527552
Total memory: 632587496
Total memory: 638647424
Total memory: 644723552    <-- it built up to 644 MB
Total memory: 6010320      <-- then GC collection reduced it to 6 MB
Total memory: 12090264
Total memory: 18153848
Total memory: 24221944
Total memory: 30290040
...

Note that this reduction of utilized memory does not release the RAM to the system, so a process viewer would still show 650+ MB allocated to the process. It is, however, available for reuse by .NET. This results in a process memory graph like this:

image

The bottom line is that the Base64QRCode class is quite inefficient, partially because it is based on the System.Drawing.Common library, causing poor performance -- but no memory leak.

The best start towards solving this problem would be to add a benchmarking project to QRCoder relying on the BenchmarkDotNet library, and measure the exact amount of memory that gets allocated during generation of the QR code. There are many ways that the amount of allocations could easily be reduced. For example, instead of executing memoryStream.ToArray() and then converting the array to Base64, it could use GetBuffer() and convert the utilized segment of the memory stream's buffer to base64. Another benefit would be to utilize the PngByteQRCode class internally when saving as a PNG file (which is the default) instead of using the QRCode class.

Shane32 commented 3 months ago

I added PR #495 which uses the PngByteQRCode class when png files are requested, optimizing the speed and memory requirements in the default scenario, while also allowing the class to function under Linux.

codebude commented 3 months ago

The bottom line is that the Base64QRCode class is quite inefficient, partially because it is based on the System.Drawing.Common library, causing poor performance -- but no memory leak.

Thanks for the conclusion. In view of the issue title (memory leak) - which is not present as shown and the fact that with #495 there is also an option with better performance (Base64QRCode with ImageType == PNG) now, I would like to close the issue here.