Skip to content

Commit dd5e939

Browse files
authored
Throw exception instead of silently failing if zip save/close fails (#54)
I saw a case before on my machine where closing the zip would fail to write out changes, but it would fail silently, so it wasn't obvious there was a problem there. See more details here: https://xamarinhq.slack.com/archives/C03CEGRUW/p1588784048377800 This PR makes those errors explicit, throwing an exception on Close failures, so that the user will see there's a problem, that the ZIP/APK didn't actually get updated. I'm honestly a little nervous about this change - it seems good conceptually but might there be cases in practice where it's better to fail silently? I'm not sure, but I wanted to create a PR to get feedback from you all on that. As for my machine, it was a failure to create temp file error (see Slack). The problem went away after I rebooted and I wasn't able to reproduce it again. Probably it was some file in use issue of some kind. This may or may not be a problem customers see in practice (of course, since it fails silently, we have less visibility into whether it is a problem in practice - making the error explicit would help with that at least).
1 parent 2df5b16 commit dd5e939

File tree

1 file changed

+9
-5
lines changed

1 file changed

+9
-5
lines changed

Diff for: ZipArchive.cs

+9-5
Original file line numberDiff line numberDiff line change
@@ -869,12 +869,16 @@ public void Close ()
869869
if (archive == IntPtr.Zero)
870870
return;
871871

872-
Native.zip_close (archive);
873-
foreach (var s in sources) {
874-
s.Dispose ();
872+
try {
873+
if (Native.zip_close (archive) < 0)
874+
throw GetErrorException ();
875+
} finally {
876+
foreach (var s in sources) {
877+
s.Dispose ();
878+
}
879+
sources.Clear ();
880+
archive = IntPtr.Zero;
875881
}
876-
sources.Clear ();
877-
archive = IntPtr.Zero;
878882
}
879883

880884
/// <summary>

0 commit comments

Comments
 (0)