avoid leaking objects when exceptions are thrown

[zadr]

@jbmorgan pointed out that by default, under ARC, the lifetime of a variable caught in an exception does not end when a scope ends https://clang.llvm.org/docs/AutomaticReferenceCounting.html#exceptions

by making a __weak reference before entering an @try block, we can avoid leaking memory when hitting assertions around low disk space (to avoid also crashing due to low memory!)

similarly @JaviSoto pointed out that we should combine data into one object before calling write, to ensure we atomically write everything or nothing.

[JaviSoto]

Thanks! I think this may be worth doing, so at least we don't write partial data, but it still doesn't guarantee that the file won't be corrupted, since the code that uses this is probably not resilient to these writes failing (the caller doesn't even know that the call failed! Idk how critical this is, but we may want to make this method return a BOOL perhaps?

[JaviSoto]

Did you test this with optimizations enabled? I think this may not work. ARC is allowed to release combinedData right after this line, and weakCombinedData would then be nil.

[zadr]

only ran tests in debug mode

since weakCombinedData is referenced below in a nested scope, i don't think ARC would relase things immediately after this line?

[JaviSoto]

It would because it's a weak reference. Weak references don't extend their lifetime. In fact, you get a warning in Swift doing this precisely for that reason:

weak var bar = NSObject()
print(bar) // This actually prints nil!

[zadr]

i think the swift equivalent here would be something like

        let x = NSObject()
        weak var weakX = x
        autoreleasepool {
            print(String(describing: weakX))
        }

which does not result in any warnings and prints out

Optional(<NSObject: 0x6000014d80b0>)

since weakCombinedData is a weak reference to combinedData — which is also in scope right now

and since your other comment calls out how @autoreleasepool {} is equivalent to NSAutoreleasePool we know we don't have to worry about the scope going away (resulting in combinedData leaving us— at which point weakCombinedData would indeed become nil!)

[JaviSoto]

This syntax is equivalent to:

NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
[self writeData:weakCombinedData];
[pool release];

Given that writeData can throw, it's easy to see that in that case we'd leak the autorelease pool (and with that, every autoreleased object).

There's really no safe way to catch Obj-C exceptions under ARC. Even doing what I suggested, wrapping -writeData: in a file that we compile without ARC would leak objects, because the code inside Foundation's method doesn't expect that the exception is throws will be caught.
I think we'd have to be OK with leaking memory in this scenario tbh.