Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Open
onekiloparsec opened this Issue Feb 22, 2012 · 17 comments

Comments

Projects
None yet

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.

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.

Owner

ccgus commented Feb 22, 2012

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.

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 Nov 9, 2012

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.

Contributor

danieldickison commented Dec 15, 2012

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 Jan 18, 2013

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?

Owner

ccgus commented Jan 19, 2013

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 Jan 20, 2013

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 Feb 12, 2013

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 Feb 28, 2013

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 Feb 28, 2013

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 Feb 28, 2013

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 Mar 1, 2013

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

DrBeak1 commented Jun 30, 2013

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!

+1 to have async version of the block methods!

Collaborator

robertmryan commented Oct 22, 2014

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 Dec 10, 2014

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment