Memory is leaking when multithreading

[VoidXH]

For a ~2 GB 7z with ~40k files (this one specifically), running the following code will eat 64 GBs of memory in a minute:

static void Extract(string archive, string output) {
    Console.WriteLine($"Extracting {Path.GetFileName(archive)}...");
    using SevenZipArchive release = SevenZipArchive.Open(archive);
    SevenZipArchiveEntry[] toExtract = release.Entries.Where(entry => !entry.IsDirectory).ToArray();

    ExtractionOptions options = new() {
        ExtractFullPath = true,
        Overwrite = true
    };
    int done = 0;
    DateTime nextUpdate = default;
    Parallel.For(0, toExtract.Length, i => {
        Interlocked.Increment(ref done);
        SevenZipArchiveEntry entry = toExtract[i];
        string path = Path.Combine(output, entry.Key);
        if (File.Exists(path) && new FileInfo(path).Length == entry.Size) {
            return;
        }
        try {
            SevenZipArchive handle = SevenZipArchive.Open(archive);
            handle.Entries.First(x => x.Key == entry.Key).WriteToDirectory(output, options);
            handle.Dispose();
        } catch {
            Console.WriteLine($"[WARN] Couldn't extract {toExtract[i].Key}.");
        }

        if (nextUpdate < DateTime.Now || done == toExtract.Length) {
            nextUpdate = DateTime.Now + TimeSpan.FromSeconds(1);
            //ProgressBar(done, toExtract.Length);
        }
    });
    Console.WriteLine("\nExtraction completed.");
}

Note that the opening of a new archive is needed in the try block, because multithreading a single handle to the archive would make WriteToDirectory throw a lot of exceptions.

[adamhathcock]

It's not leaking if the end of the using recovers it. That said, the 7zip extraction isn't the best. Most of the code should be gone over with stackallocations or pooling.

You're using a lot of duplicate things anyway by extracting an archive then extracting it again?

I would encourage not to use 7zip as a format and stick to zip or gzip or bzip2 or anything else really.

[VoidXH]

It's not leaking if the end of the using recovers it. That said, the 7zip extraction isn't the best. Most of the code should be gone over with stackallocations or pooling.

The end of the using doesn't recover it, all handles are disposed just after use. If this runs single-threaded, the issue is not there. I assume this might be because of some static references.

You're using a lot of duplicate things anyway by extracting an archive then extracting it again?

No, it's just opening the archive again for each extracted file. They only get extracted once. This is needed because if you use the same SevenZipArchive object from two threads, one WriteToDirectory will throw an exception. It only works if you use one object for one write.

I would encourage not to use 7zip as a format and stick to zip or gzip or bzip2 or anything else really.

7z was chosen by the linked dependency.

[VoidXH]

The leak is not caused by the disposal, but doing multithreaded export. The following code also leaks, even if it reuses the handles:

public class SevenZipArchivePool(string archive) : IDisposable {
    readonly Stack<SevenZipArchive> handles = new();
    readonly object locker = new();

    public SevenZipArchive Lease() {
        lock (locker) {
            if (handles.Count == 0) {
                return SevenZipArchive.Open(archive);
            } else {
                return handles.Pop();
            }
        }
    }

    public void Return(SevenZipArchive handle) {
        lock (locker) {
            handles.Push(handle);
        }
    }

    public void Dispose() {
        while (handles.Count != 0) {
            handles.Pop().Dispose();
        }
    }
}

static void Extract(string archive, string output) {
    Console.WriteLine($"Extracting {Path.GetFileName(archive)}...");
    using SevenZipArchivePool pool = new(archive);
    SevenZipArchive handle = pool.Lease();
    SevenZipArchiveEntry[] toExtract = handle.Entries.Where(entry => !entry.IsDirectory).ToArray();
    pool.Return(handle);

    ExtractionOptions options = new() {
        ExtractFullPath = true,
        Overwrite = true
    };
    DateTime nextUpdate = default;
    int done = 0;
    Parallel.For(0, toExtract.Length, i => {
        Interlocked.Increment(ref done);
        SevenZipArchiveEntry entry = toExtract[i];
        string path = Path.Combine(output, entry.Key);
        if (File.Exists(path) && new FileInfo(path).Length == entry.Size) {
            return;
        }

        SevenZipArchive handle = pool.Lease();
        try {
            handle.Entries.First(x => x.Key == entry.Key).WriteToDirectory(output, options);
        } catch {
            if (!File.Exists(path) || new FileInfo(path).Length != entry.Size) {
                Console.WriteLine($"[WARN] Couldn't extract {toExtract[i].Key}.");
            }
        }
        pool.Return(handle);

        if (nextUpdate < DateTime.Now || done == toExtract.Length) {
            nextUpdate = DateTime.Now + TimeSpan.FromSeconds(1);
            ProgressBar(i, toExtract.Length);
        }
    });
    Console.WriteLine("\nExtraction completed.");
}

[Morilli]

This "leak" might be caused by you not properly Disposeing the SevenZipArchive created:

        try {
            SevenZipArchive handle = SevenZipArchive.Open(archive);
            handle.Entries.First(x => x.Key == entry.Key).WriteToDirectory(output, options);
            handle.Dispose();
        } catch {
            Console.WriteLine($"[WARN] Couldn't extract {toExtract[i].Key}.");
        }

You should either write using SevenZipArchive handle = SevenZipArchive.Open(archive); or dispose the archive in a finally block, NOT in the try block, as that code will not get hit when an exception occurs.

I can not reproduce any memory issues with either the first or second example you posted however. Note that each archive instance will allocate 64MB for the window, so if the parallel code uses 64 threads you're gonna get 64MB*64 = 4GB memory usage.

The most glarious issue here however is the fact that the archive you're trying to extract is a solid archive, meaning you should be using ExtractAllEntries on the SevenZipArchive and iterate over the entries using the reader returned from that function, otherwise extraction performance will be abysmal.
Something like this:

        using var reader = release.ExtractAllEntries();
        while (reader.MoveToNextEntry())
        {
            reader.WriteEntryToDirectory(output, options);
        }

That said, there is still an issue where SevenZipFilePart.GetCompressedStream will throw an exception when the file to be extracted is empty (#854), which will cause a lot of exceptions which may be the cause of the perceived memory leak.

[VoidXH]

You should either write using SevenZipArchive handle = SevenZipArchive.Open(archive); or dispose the archive in a finally block, NOT in the try block, as that code will not get hit when an exception occurs.

This was fixed in the second snippet.

The most glarious issue here however is the fact that the archive you're trying to extract is a solid archive, meaning you should be using ExtractAllEntries on the SevenZipArchive and iterate over the entries using the reader returned from that function, otherwise extraction performance will be abysmal.

Thanks, I'll try with this one, and if it works with fine performance, I'll close this issue.

[VoidXH]

Yup, performance is fine, thanks.

[adamhathcock]

Thanks @Morilli