square / Aardvark

Aardvark is a library that makes it dead simple to create actionable bug reports.
Apache License 2.0
260 stars 41 forks source link

ARK_writeDataBlock uses deprecated writeData that throws Exception #74

Open eikefalkenberg opened 5 years ago

eikefalkenberg commented 5 years ago

There are several calls to NSFileHandle's writeData inNSFileHandle+ARKAdditions.m which is deprecated.

API_DEPRECATED_WITH_REPLACEMENT("writeData:error:",
                                    macos(10.0, API_TO_BE_DEPRECATED), ios(2.0, API_TO_BE_DEPRECATED),
                                    watchos(2.0, API_TO_BE_DEPRECATED), tvos(9.0, API_TO_BE_DEPRECATED));

The problem here is that this old method will raise an exception in case of an error, especially if the device runs out of disk space:

If the receiver is a file, writing takes place at the file pointer’s current position. After it writes the data, the method advances the file pointer by the number of bytes written. **This method raises an exception if the file descriptor is closed or is not valid, if the receiver represents an unconnected pipe or socket endpoint, if no free space is left on the file system, or if any other writing error occurs.

Using the newer writeData:error: selector would return an error instead of causing an exception.

dfed commented 5 years ago

I'm working on creating a modernized NSFileHandle+ARKAdditions in https://github.com/dfed/CacheAdvance/pull/1, though I'm not sure if the current owners of this library would want to introduce that dependency.

One complication (which I haven't yet solved in CacheAdvance) is that the new methods are iOS 13+, so we'd need to continue using the old methods in iOS 12, though we could wrap the old methods and have them return an error (or throw in Swift).

eikefalkenberg commented 5 years ago

Sounds great! The problem for this one in particular is that it runs asynchronous on it's own queue, so you can't handle the exception or a return error. A low hanging fruit here would be to just ignore the error with the new api.

eikefalkenberg commented 5 years ago

Oh sorry now I see it, I missed that it's a new method, so we might have to wrap it in something like

if (@available(iOS 13.0, *)) {
        NSError *error;
        [self writeData:dataLengthData error:&error];
        [self writeData:dataBlock error:&error];
    } else {
        @try {
            [self writeData:dataLengthData];
            [self writeData:dataBlock];
        }
        @catch ( NSException *e ) {
        }
    }