starred/sharpcompress
1
0
Fork 0
mirror of https://github.com/adamhathcock/sharpcompress.git synced 2026-02-03 21:23:38 +00:00
Code Issues 975 Packages Projects Releases 20 Wiki Activity

Memory is leaking when multithreading #673

New Issue
Closed
opened 2026-01-29 22:15:35 +00:00 by claunia · 7 comments
claunia commented 2026-01-29 22:15:35 +00:00
Owner
Copy Link

Originally created by @VoidXH on GitHub (Feb 17, 2025).

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.

Originally created by @VoidXH on GitHub (Feb 17, 2025). For a ~2 GB 7z with ~40k files ([this one specifically](https://github.com/AUTOMATIC1111/stable-diffusion-webui/releases/download/v1.10.0/sd.webui-1.10.1-blackwell.7z)), running the following code will eat 64 GBs of memory in a minute: ```csharp 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.
claunia added the bugup for grabs labels 2026-01-29 22:15:35 +00:00
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@adamhathcock commented on GitHub (Feb 18, 2025):

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.

@adamhathcock commented on GitHub (Feb 18, 2025): 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.
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@VoidXH commented on GitHub (Feb 18, 2025):

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 commented on GitHub (Feb 18, 2025): > 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.
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@VoidXH commented on GitHub (Feb 18, 2025):

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.");
}
@VoidXH commented on GitHub (Feb 18, 2025): The leak is **not** caused by the disposal, but doing multithreaded export. The following code also leaks, even if it reuses the handles: ```csharp 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."); } ```
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@Morilli commented on GitHub (Feb 19, 2025):

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.

@Morilli commented on GitHub (Feb 19, 2025): This "leak" might be caused by you not properly `Dispose`ing the `SevenZipArchive` created: ```cs 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: ```cs 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.
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@VoidXH commented on GitHub (Feb 19, 2025):

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 commented on GitHub (Feb 19, 2025): > 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.
claunia commented 2026-01-29 22:15:36 +00:00
Author
Owner
Copy Link

@VoidXH commented on GitHub (Feb 19, 2025):

Yup, performance is fine, thanks.

@VoidXH commented on GitHub (Feb 19, 2025): Yup, performance is fine, thanks.
claunia commented 2026-01-29 22:15:37 +00:00
Author
Owner
Copy Link

@adamhathcock commented on GitHub (Feb 20, 2025):

Thanks @Morilli

@adamhathcock commented on GitHub (Feb 20, 2025): Thanks @Morilli
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
duplicate
enhancement
feedback requested
not bug
pull-request
Mirrored from GitHub Pull Request
 question
up for grabs
wontfix
No Label bug up for grabs
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/sharpcompress#673
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?