FMResultSet (lack of) error handling

[snej]

I'm concerned about the way FMResultSet handles errors from sqlite3_step (the call is here.) It looks as though the response to an error is simply to return NO from -next. That means the caller can't tell the difference between an error and a normal end-of-data.

Specifically, I think SQLITE_BUSY or SQLITE_LOCKED could come up in normal multithreaded use, and need to be handled by backoff and retry; but instead the caller will just get zero rows of results and continue on. The consequences could be really bad.

[snej]

Here's how I'd suggest fixing this: The initial sqlite3_step call should be made at the time the FMResultSet is created, i.e. during the -executeQuery:... call. That way -executeQuery: can return nil, which will indicate to the caller that the query failed and it should check the lastErrorCode property.

It's my understanding that, once the first sqlite3_step call from a SELECT query succeeds, the connection will have acquired a read-lock on the database and there's no more chance of a busy/locked error from that result set.

[robertmryan]

I can't speak for Gus, but my reaction is that in "normal multithreaded use", you never get SQLITE_BUSY or SQLITE_LOCKED if you use FMDatabaseQueue properly. Thus, in proper usage, errors in sqlite3_step are exceedingly unlikely.

Having said that, I agree in principle that next really should differentiate between an error and SQLITE_DONE.

There are a couple of possible patterns here that might be suitable:

1. We could have a rendition of next that takes a NSError * __autoreleasing * parameter and if not nil, fills it with the errorWithMessage results.
2. We could use a block-based enumeration method that would be called as you iterate through the result set's rows.

While I naturally gravitate to the latter solution (I love blocks and there's something mildly discomfiting about having to litter my code with FMResultSet code where I iterate through a while ([rs next]) loop), but the former is probably simplest.

[snej]

Ah, I'm on a custom fork of FMDB that doesn't have FMDatabaseQueue. Instead I make each thread open its own FMDatabase instance.

[snej]

Oh, also, even with FMDatabaseQueue it's possible to get SQLITE_BUSY errors … if there's another process accessing the database. Not the most likely scenario, but one that can arise if a database is being used as a shared store between two apps, or between an app and its background helper process, or something like that. (The PubSub framework on OS X does this, since the user's RSS article database is shared between client apps and the background agent that updates feeds.)

[ccgus]

I agree that the error handling needs to be better- especially with busy errors. (I recently committed a little note to myself on that as well: https://github.com/ccgus/fmdb/commit/99bf192e1ef7986d5cdcbb895987e121a7f24925 )

FMDB used to do a little loop there, but I took it out when I added a method which calls sqlite3_busy_timeout. I mistakenly thought that it would retry automatically (I was wrong).

A block new based approach for going through the results like Rob suggests would be a good way to move forward I think. I'll see if I can come up with something relatively soonish.

[ccgus]

So as part of this - I'm adding back in the loops which check if the database is busy or locked, and then retrying the previous sqlite_step if it is. The question is - how do I break out of this if it's locked "forever"? Previously I just kept a counter, but that seems wrong to me. Another solution is to keep track of time and break after x number of seconds.

Do either of you have an opinion on this? Here's the bit of code that I'm thinking of: https://github.com/ccgus/fmdb/commit/995dbb5540d3431decb10de6eff51a79c86527c8#diff-c68bf406594a6660e726ff51d11fb527R962

[robertmryan]

Personally, it pains me a little to see us do that (because proper use FMDatabaseQueue should mean that you never get this error except in those atypical scenarios where you're accessing a database that other processes might also be interacting with). 99% of the "how do I handle SQLITE_BUSY?" questions I see on Stack Overflow are not legitimate scenarios where multiple processes are trying to access the same database, but rather sloppy handling of sqlite pointers or FMDatabase instances within a multithreaded app.

But to handle that edge case (where multiple processes need to access the same database), some retry logic like what you've proposed here is probably prudent. I agree with you that the number of retries counter is not right. I'd suggest a retryTimeout property (like you see in networking API's) that specifies a CFTimeInterval or NSTimeInterval number of seconds to retry if busy. (I think a timeout parameter is better than a max number of retries, because the latter is hardware specific, and generally a good app developer is more concerned about user experience than number of iterations in some while loop.)

Having said that, because of my rant in the my first paragraph, I think this retryTimeout should default to zero. We probably don't want app developers relying on some default non-zero timeout parameter to cover-up architectural design flaws in their app. Sure, provide it if they really need it, but don't default it to being on. And the documentation for retryTimeout should bear a @warning that makes it clear that this is only intended for those rare multi-process scenarios, and that generally one should use FMDatabaseQueue to coordinate database interaction within an app, completely obviating the need for retryTimeout.

A few other thoughts on error handling:

1. Given that you still need some timeout logic (and thus next may fail), I think we really need to address the "how do I differentiate between some sqlite3_step error (such as busy database) and end of record set?"

   I'd still vote for a block-based method for enumerating through the result set (the Cocoa analogy that leaps to mind is enumerateMatchesInString:options:range:usingBlock: method of NSRegularExpression that has a BOOL * which the API developer can set to stop the enumeration). And obviously the block parameter should also pass a NSError object (like NSURLConnection and NSURLSession block-based methods do) or a separate dedicated failure block (like AFNetworking seems to prefer).

   Alternatively, you could have a rendition of next that returns an autoreleased NSError object (like much of the traditional cocoa API does, e.g. NSFileManager methods).

2. To broaden this conversation where I'm sure you didn't want to go, I'd actually suggest that the entire API should have renditions of the methods that return NSError objects.

   Conceptually, it seems that anything you might log if logsErrors is YES should really be returned in a NSError object via the API. It seems fundamentally wrong for there to be information that we might log in a console, but not provide to the app developer programmatically. Sure, unlike next method, most methods are returning a BOOL which may prompt the developer to inquire with lastErrorMessage, returning a NSError object seems more elegant.

3. This goes to the broader NSLog philosophy of the framework. Personally, I don't think that logsErrors should default to YES. I'd suggest changing it so that it defaults to YES if #ifdef DEBUG and NO otherwise (to prevent app developers from accidentally shipping production code that does NSLog statements that they didn't write themselves). Sure, if app developers who need this logging manually set logsErrors to be whatever they want, but if you're going to default it to YES (which I'm not crazy about anyway), maybe only do that if it's a #ifdef DEBUG build.

I apologize for the digression on broader logging and error handling, but that's my two cents (and even at that, I'm not sure you got your money's worth).

[ccgus]

Personally, it pains me a little to see us do that (because proper use FMDatabaseQueue should mean that you never get this error except in those atypical scenarios where you're accessing a database that other processes might also be interacting with).

Well, I personally want it, because I've had situations where two apps where hitting the same file on OS X. Plus, it'll make all the tests pass :)

But to handle that edge case (where multiple processes need to access the same database), some retry logic like what you've proposed here is probably prudent. I agree with you that the number of retries counter is not right. I'd suggest a retryTimeout property (like you see in networking API's) that specifies a CFTimeInterval or NSTimeInterval number of seconds to retry if busy.

I'll see if I can add that in.

Having said that, because of my rant in the my first paragraph, I think this retryTimeout should default to zero.

I agree.

A few other thoughts on error handling:

I'd still vote for a block-based method for enumerating through the result set (the Cocoa analogy that leaps to mind is enumerateMatchesInString:options:range:usingBlock: method of NSRegularExpression that has a BOOL * which the API developer can set to stop the enumeration). And obviously the block parameter should also pass a NSError object (like NSURLConnection and NSURLSession block-based methods do) or a separate dedicated failure block (like AFNetworking seems to prefer).

Personally, I'd rather FMDB pass an NSError back.

Alternatively, you could have a rendition of next that returns an autoreleased NSError object (like much of the traditional cocoa API does, e.g. NSFileManager methods).

That's not bad.

To broaden this conversation where I'm sure you didn't want to go, I'd actually suggest that the entire API should have renditions of the methods that return NSError objects.

I agree, but my hangup is, what does FMDatabase's executeQuery: look like now? The vargs has to come last, and I don't want to make a new query object that gets passed it. Things need to be simple, and unugly. This is super ugly to me:

• (FMResultSet _) error:(NSError_)outErr executeQuery:(NSString)sql, ...;

But what other choice is there?

Conceptually, it seems that anything you might log if logsErrors is YES should really be returned in a NSError object via the API. It seems fundamentally wrong for there to be information that we might log in a console, but not provide to the app developer programmatically. Sure, unlike next method, most methods are returning a BOOL which may prompt the developer to inquire with lastErrorMessage, returning a NSError object seems more elegant.

This goes to the broader NSLog philosophy of the framework. Personally, I don't think that logsErrors should default to YES. I'd suggest changing it so that it defaults to YES if #ifdef DEBUG and NO otherwise (to prevent app developers from accidentally shipping production code that does NSLog statements that they didn't write themselves). Sure, if app developers who need this logging manually set logsErrors to be whatever they want, but if you're going to default it to YES (which I'm not crazy about anyway), maybe only do that if it's a #ifdef DEBUG build.

That's reasonable.

Maybe all this should be done in an FMDB version 3…

[robertmryan]

Good observations, throughout.

As an aside you note that the following is ugly:

- (FMResultSet *)error:(NSError * __autoreleasing *)error executeQuery:(NSString *)sql, ...

Agreed; that seems backwards. Maybe those methods with variadic arguments should be excepted from the pattern. Or maybe my notion doesn't stand up to scrutiny. :)

Frankly, I'd probably encourage the use of a NSArray permutation of executeQuery:

- (FMResultSet *) executeQuery:(NSString *)sql parameters:(NSArray *)parameters error:(NSError * __autoreleasing *)error

If you look at Cocoa frameworks, in their newer additions generally use array based parameters (e.g. see UIKit Dynamics API).

[ccgus]

I wonder if I could just add an **NSError at the end of the vargs, and if FMDB notices that there's one more argument than there should be, it'll use that? I'll have to try and see if that works.

[robertmryan]

I'm not sure it will. As the va_arg documentation says:

If there is no next argument, or if type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), random errors will occur.

Apple has a technical document that suggests you can do that (QA1405), but in my experience, if you try to call va_arg when there's nothing there, you get errors (I experienced EXC_BAD_ACCESS when I tried it).