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

Replace DotNetZip with System.IO.Compression #862

Merged
merged 3 commits into from
Feb 2, 2025

Conversation

maddie480
Copy link
Member

Someone recently ran into this:

System.ObjectDisposedException: Cannot access a closed Stream.
   at System.IO.Strategies.BufferedFileStreamStrategy.Seek(Int64 offset, SeekOrigin origin)
   at Celeste.Mod.ZipModContent.OpenParaEntry(ZipEntry real) in /home/vsts/work/1/s/Celeste.Mod.mm/Mod/Everest/Everest.Content.cs:line 448
   at Celeste.Mod.Helpers.ZipAssetStream..ctor(ZipModAsset asset, ZipModSecret secret) in /home/vsts/work/1/s/Celeste.Mod.mm/Mod/Helpers/ZipAssetStream.cs:line 28
   at Celeste.Mod.ZipModAsset.Open(Stream& stream, Boolean& isSection) in /home/vsts/work/1/s/Celeste.Mod.mm/Mod/Everest/ModAsset.cs:line 317
   at Celeste.Mod.ModAsset.get_Stream() in /home/vsts/work/1/s/Celeste.Mod.mm/Mod/Everest/ModAsset.cs:line 47
   at Celeste.Mod.AurorasHelper.PlayAudioTrigger.Added(Scene scene) in D:\Games\SteamLibrary\steamapps\common\Celeste\Mods\AurorasHelper\Triggers\PlayAudioTrigger.cs:line 206
[...]

... which looks like a race condition in zip file accessing to me. And since the zip handling is quite ... complex, with pools of zip file streams and reflection to access DotNetZip internals, I'm trying to replace it with the System.IO.Compression implementation.

Points to watch out for:

  • I'm creating MemoryStreams with file contents when they are requested, because ZipArchive doesn't seem thread-safe (I've got "unsupported compression method" when I tried), does this use way too much RAM?
  • Is the deletion of the zip pool thing harmful for startup speed? Files are read one at a time from each zip, instead of having the zip opened 4 times to read 4 files at once.
  • Do some mods explode? At least having Spring Collab, Strawberry Jam and CelesteTAS enabled didn't.

@maddie480
Copy link
Member Author

My mighty grep on the mirror pulled out 3 DLLs that contain the string "DotNetZip": Gramophone, PlattenTek and Infinite Backups... checking what this is about now

@maddie480
Copy link
Member Author

Gramophone 2.x crashes with an assertion failure, which makes me think I should make 1.0.5 the latest version on the updater 🤔

anyway, seems like continuing to ship DotNetZip is enough to make those mods work

@maddie480
Copy link
Member Author

Startup timing with Spring Collab + Strawberry Jam + dependencies enabled and Cache folder deleted, Celeste being installed on an NVMe SSD:

  • this branch: 1 minute 36 seconds
  • latest dev: 2 minutes 14 seconds

... huh 🤔

RAM usage seems around the same (looking at System Monitor)

@maddie480
Copy link
Member Author

same with a Cache folder already built:

  • latest dev: 46 seconds
  • this branch: 40 seconds

so it definitely doesn't hurt performance

@microlith57
Copy link
Member

microlith57 commented Dec 29, 2024

re. memory stream: it would probably be better at least look further into ZipArchiveEntry.Open; i think the relevant error would be coming from https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5ac14/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs#L732

@maddie480
Copy link
Member Author

maddie480 commented Dec 29, 2024

https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs#L438

the existence of an "archive stream owner" lets me think we can't read from multiple entries at the same time anyway 🤔 they're going to be fighting over the archive stream position otherwise, and I think that's why the "invalid compression method" thingy happens: thread 1 seeks elsewhere, thread 2 reads garbage and freaks out

@microlith57
Copy link
Member

oh so there's only one underlying stream, right
unfortunate but i guess that's reasonable

@maddie480 maddie480 merged commit 41b7e39 into EverestAPI:dev Feb 2, 2025
1 check passed
@maddie480 maddie480 deleted the kick-out-dotnetzip branch February 2, 2025 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants