ccgus / fmdb

A Cocoa / Objective-C wrapper around SQLite
Other
13.85k stars 2.77k forks source link

An "async" version of "inTransaction:" ? + a design question #47

Open onekiloparsec opened 12 years ago

onekiloparsec commented 12 years ago

Hi. I was about to implement the FMDatabaseQueue stuff myself before I discovered it's just been added to fmdb. But I see all the transactions are done with a "dispatch_sync" which doesn't return until all the stuff is made.

In my code I would need an "async" version of it. That is, schedule the block to the serial queue, and move on immediately. The crucial point is the serial nature of the queue to avoid multi-threaded collisions. But not the way you schedule blocks to it.

The implementation of the async version could virtually be identical to the sync one, apart from the dispatch function, and the absence of need to make a retain/release (for non-arc code), since a Block_copy() is made in dispatch_aync.

To the contrary of the sync version however, there could be some difficulties if you need to perform some actions once the block is done on success, or when it has failed. We can imagine other block arguments, that could be performed on the main global queue (just a suggestion) after the rollback / commit.

Also, I wonder the meaning of passing "db" as an argument of the block of transactions, and always calling [self database] inside the method implementation. What happens if they are different?

The example given in the README of fmdb is confusing. Imagine you have another "FMDatabase" called db2. Nothing prevent you to write (it's a mistake I agree), inside the "inTransaction" block: "[db2 executeUpdate:...]". Then the whole transaction is meaningless.

In fact, to me, the "inTransaction" method should be a method of the FMDatabase instance itself. In fact, instead of a new class FMDatabaseQueue, every "standard" update and query should be done on the underlying serial queue in the FMDatabaseInstance, and then the whole FMDB lib would be thread-safe thanks to GDB.

My 2-cents.

onekiloparsec commented 12 years ago

Just thinking again on what I wrote: a method "inTransaction" taking a block as argument whose arguments comprises or not a FMDatabase instance makes no sense either. We would have :

[db inTransaction:^(FMDatabase db) { [db executeQuery:...]; }];

Sorry for the confusion. The only interesting "pattern" I see for our case here would be the class/static methods used to make animations with UIView. But this makes other complications as well.

ccgus commented 12 years ago

I would suggest coding up what the async version would look like, and see if it suits your purposes as well as you think. The idea of passing two blocks though- that sounds a bit cumbersome.

I'm not sure this would be of use to most folks though- and it might just be easier to implement your async queue outside of FMDatabaseQueue.

onekiloparsec commented 12 years ago

I agree that passing two blocks is a bit cumbersome (even if it is done in UIView class methods, and being extremely useful).

For the moment, I have only a custom class containing side by side a queue and a DB (with a little FMDatabase category). And my async method looks like this:

- (void)performAsyncDBTransaction:(dispatch_block_t)block onFailure:(dispatch_block_t)failureBlock
{
    dispatch_async(queue, ^{
        [db beginTransaction];
        @try {
            block();
            [db throwOnError];
            [db commit];
        }
        @catch (NSException *exception) {
            [db rollback];
            dispatch_async(dispatch_get_main_queue(), failureBlock);
        }
    });
}

(There is an additional handling of exceptions in this code, which is not the purpose of the comment).

Remains however the question to me of the FMDatabase "db" argument versus the [self database] calls in your implementation. But I don't have smart suggestions for now. Thanks anyway for your work, it is indeed of very much help.

mxcl commented 12 years ago

I was expecting the blocks to be async, I'm not sure why they aren't, but can I suggest you at least document this on the functions. The current README says it creates the "GCD queue in the background" and I foolishly took this to mean that the blocks would be async—I know this was a leap.

Love the library otherwise, etc. etc.

danieldickison commented 11 years ago

Just another vote for switching the in* methods to use dispatch_async, which I think is more intuitive in this case. But it's not hard to work around the current sync methods by wrapping the call inside of an async dispatch:

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
    [dbQueue inDatabase:^(FMDatabase *db) {
        ...
    }];
});
nicked commented 11 years ago

I also found it confusing that the documentation says "in the background" yet dispatch_sync is used, which will run database queries on the main thread.

Would there be any side effects of just changing the dispatch_sync calls to dispatch_async?

ccgus commented 11 years ago

I've updated the documentation to take out the "background" wording.

@nicked There would be huge side effects to changing it to dispatch_async. For one, everything would then be asynchronous…

nicked commented 11 years ago

Maybe I'm missing something obvious, but what's the point of using blocks then? If you wanted to query the database from multiple threads synchronously, couldn't the executeQuery etc calls block execution and add themselves to a serial queue?

Using blocks suggests that FMDatabaseQueue works asynchronously, and that you should be sending a notification or something once your data actually loads.

Anyway no dramas, I've wrapped all my inDatabase calls in async blocks now. Clarifying the doco should help avoid future confusion, thanks :)

Cheers, Nick

ghoover commented 11 years ago

Both versions make sense for different situations. Sometimes you want the application to block until saves are complete (like when exiting an application). Other times, it doesn't matter as long as it gets done. It can be quite a bit easier to maintain a responsive UI if you can push long running saves off of the main thread. How about adding methods to suite the async case?

inDatabaseAsync:
inTransactionAsync:

Or add an async argument to inDatabase: / inTransaction:

Just an idea.

I ended doing almost exactly this several years ago and found FMDB while debating to migrate to CoreData. Looks like a great API.

Greg

haikusw commented 11 years ago

I definitely need async queries with callback blocks. Passing multiple blocks is fine (the block to execute and the block to execute on completion).

mxcl commented 11 years ago

It's pretty simple to make your own. Just make an NSOperationQueue with a concurrency count of 1. You can add some sugar so you can pass completion blocks and have them automatically called, using the completionBlock feature of NSOperations.

haikusw commented 11 years ago

Interesting. I'm still wrapping my head around gcd but I thought it mostly meant NSOperationQueues weren't that useful for the most part... I'm curious why one wouldn't just do async dispatch to a serial dispatch queue? Not sure if that's an appropriate discussion for this thread though (not sure on etiquette here). Maybe in google group would be better?

cse190 commented 11 years ago

NSOperation and NSOperationQueue use GCD internally so you get the inherent benefits plus the ease of use.

DrBeak1 commented 11 years ago

I love FMDB, it has made my life so much easier! That said, having an async version of the block methods would be icing on the cake.

Thanks for everything!

loretoparisi commented 10 years ago

+1 to have async version of the block methods!

robertmryan commented 10 years ago

I agree that FMDatabaseQueue should offer asynchronous renditions. By definition, one uses FMDatabaseQueue in those situations where there are multiple threads contending for the shared resource of the database. Therefore calls to inDatabase and inTransaction can block. But we should never block the main thread. We should take a page from Apple's book: In the Cocoa API, if a method might potentially block, Apple provides asynchronous rendition (and, in fact, if you look at their newer APIs, they often simply don't provide synchronous renditions!).

By the way, this is a wonderful time to deprecate the inDatabase and inTransaction names, too. I'd suggest something like addBlock and addTransactionBlock, which implicitly make it clear that the method is adding something to a queue that will be performed asynchronously.

mirion commented 9 years ago

+1