Assertion When Dispatching propertiesToSave While Running Kiwi Tests

[David Quon]

I changed my code from:

properties = self.propertiesToSave; 

To something like this ensuring I'm always accessing the database on the same thread:

dispatch_sync(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

        properties = self.propertiesToSave;

});

Now this works fine for running the app on the device.  However when running the Kiwi Test framework I now get Assertions like below:

(lldb) po self.propertiesToSave

2014-04-01 14:05:01.745 XXXXApp[78999:90b] *** Assertion failure in -[CBL_FMDatabase beginUse], /Users/couchbase/jenkins/workspace/build_cblite_ios_stable/couchbase-lite-ios/vendor/fmdb/src/FMDatabase.m:907

error: warning: couldn't get cmd pointer (substituting NULL): no variable named '_cmd' found in this frame

Execution was interrupted, reason: breakpoint 1.3.

The process has been returned to the state before expression evaluation.

2014-04-01 14:15:46.827 XXXXApp[78999:90b] *** Assertion failure in -[CBL_FMDatabase beginUse], /Users/couchbase/jenkins/workspace/build_cblite_ios_stable/couchbase-lite-ios/vendor/fmdb/src/FMDatabase.m:907

I did some investigation into both Couchbase Lite and FMDatabase but can't seem to pinpoint the root cause of the problem.  I just wanted to see if anyone else has seen similar issues or just get help in narrowing down the problem.

[David Quon]

Also it should be stated that I'm getting this assertion when the app is just being brought up to run.  I've disabled all of the unit test specs from running.

[David Quon]

The code changes I made should be fine regardless which version of Couchbase LIte is used and I noticed this code works fine on Couchbase Lite Beta 2 and breaks with Beta 3.

[Jens Alfke]

On Apr 1, 2014, at 3:21 PM, David Quon <dav...@amcoonline.net> wrote:

I changed my code from:

properties = self.propertiesToSave; 

To something like this ensuring I'm always accessing the database on the same thread:

dispatch_sync(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

        properties = self.propertiesToSave;

});

That’s going to invoke your block on a concurrent dispatch queue, i.e. one that can execute many blocks in parallel. I think what you intended was to invoke it on the main queue/thread? For that you’d use dispatch_get_main_queue instead. You should never run CBL code on a concurrent queue.

Also, I assume  you’re calling that from a background thread? If so, that implies a method of your CBLModel is being called on that background thread, which is already a bad idea. All calls to CBL objects should be from the thread/queue that created them.

2014-04-01 14:05:01.745 XXXXApp[78999:90b] *** Assertion failure in -[CBL_FMDatabase beginUse], /Users/couchbase/jenkins/workspace/build_cblite_ios_stable/couchbase-lite-ios/vendor/fmdb/src/FMDatabase.m:907

That’s an exception because you’re invoking CBL on the wrong thread.

error: warning: couldn't get cmd pointer (substituting NULL): no variable named '_cmd' found in this frame

Execution was interrupted, reason: breakpoint 1.3.

That’s some kind of internal error from the debugger.

—Jens

[Jens Alfke]

On Apr 1, 2014, at 5:07 PM, David Quon <dav...@amcoonline.net> wrote:

I noticed this code works fine on Couchbase Lite Beta 2 and breaks with Beta 3.

That’s because I added much more rigorous thread-safety checks in beta 3, because I was tired of all the questions about “why do I get this database-is-busy warning?” ;-)

—Jens

[David Quon]

Thanks for the response Jens.  That's what I was suspecting was happening was a threading issue.  I think I fixed the threading issues I was having in the app by doing a dispatch_sync on a particular thread.  This should make CBL happy as it's always getting written to from the same thread but the synchronous dispatch should allow the code to wait for the block to return.  For the most part the code works fine when running the app on the device.

I tried changing the thread to be the main thread if running Kiwi Unit Tests and a DB thread if running the app on the device.  However for whatever reason this just froze the app when trying to get started to run the Unit Tests (not yet running any tests).  I haven't exhausted exploring the main thread route yet but it seems logical that the app may be blocked if the tests are also trying to run on the main thread.

I'm wondering now if Kiwi Unit Tests are doing something funny like running different parts of the app in different threads.  I would have thought they would just start up the app just like the simulator or device would and then potentially run the tests using different threads.  However I can't even get the app to load properly in Kiwi now as I disabled all of the test specs.  If anyone has any insight into this please let me know.  Thanks again Jens for the help.

[Jens Alfke]

On Apr 1, 2014, at 6:00 PM, David Quon <dav...@amcoonline.net> wrote:

> I tried changing the thread to be the main thread if running Kiwi Unit Tests and a DB thread if running the app on the device. However for whatever reason this just froze the app when trying to get started to run the Unit Tests (not yet running any tests).

Well, if you call dispatch_sync from the same dispatch queue you’re dispatching to, you’ll deadlock. For example, if you’re on the main thread and try to dispatch to the main thread. That might be what’s happening.

—Jens

[David Quon]

> Well, if you call dispatch_sync from the same dispatch queue you’re dispatching to, you’ll deadlock. For example, if you’re on the main thread and try to dispatch to the main thread. That might be what’s happening. 

Ah that must be exactly what's happening as it does deadlock.  So I guess that's out as a solution.  My next steps I guess are to investigate how Kiwi handles threading and why it differs from running the app on the device.  Thanks Jens for the advice.

[David Quon]

Hi Jens.  So I put a breakpoint in the block above on properties = self.propertiesToSave; and output the following.

(lldb) po dispatch_get_current_queue()

<OS_dispatch_queue_root: com.apple.root.default-priority[0x4946100] = { xrefcnt = 0x80000000, refcnt = 0x80000000, suspend_cnt = 0x0, locked = 1, target = [0x0], width = 0x7fffffff, running = 0x2, barrier = 0 }>

(lldb) po CBLManager.sharedInstance.dispatchQueue

<OS_dispatch_queue_root: com.apple.root.default-priority[0x4946100] = { xrefcnt = 0x80000000, refcnt = 0x80000000, suspend_cnt = 0x0, locked = 1, target = [0x0], width = 0x7fffffff, running = 0x2, barrier = 0 }>

So I'm a little confused why the test ok = dispatch_get_current_queue() == dispatchQueue; in beginUse would fail.  Do you have any thoughts of what else I could be doing incorrectly to cause this assertion?

[David Quon]

Also as far as I can tell the app should be running on the main thread with the exception of the CBL communication.  In AppDelegate I set:

CBLManager.sharedInstance.dispatchQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);

Then every time I want to talk with the database I do something like this:

dispatch_sync(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

        // CBL Communication here

});

Is there anything wrong with this approach?  It seems to work fine on the device.  I'm just encountering the problem above when trying to get the Kiwi Unit Tests to work.

[Jens Alfke]

On Apr 2, 2014, at 12:43 PM, David Quon <dav...@amcoonline.net> wrote:

So I'm a little confused why the test ok = dispatch_get_current_queue() == dispatchQueue; in beginUse would fail.  Do you have any thoughts of what else I could be doing incorrectly to cause this assertion?

Well, it sounds like you’re looking at the values in one place in the code, but that’s not where the assertion is being checked. Can you post a backtrace from the exception where the assertion fails? That should show what thread/queue it's on at the time.

—Jens

[David Quon]

I emailed you the backtrace from the exception.  Thanks for your help Jens.

[David Quon]

After looking over the backtrace I emailed you I it still looks to be running on the correct thread as far as I can tell (com.apple.root.default-priority).  The only thing I see questionable in the back trace is the unavailable on the following two lines:

  * frame #3: 0x0dbb4ad4 KiwiUnitTest`-[CBL_FMDatabase beginUse](self=0x0c8a5060, _cmd=<unavailable>) + 187 at FMDatabase.m:904
    frame #4: 0x0dbb3d35 KiwiUnitTest`-[CBL_FMDatabase executeQuery:withArgumentsInArray:orVAList:](self=<unavailable>, _cmd=0x0455520c, sql=<unavailable>, arrayArgs=0x0455520c, args=0xbfffbffc) + 195 at FMDatabase.m:515

Let me know if I'm missing something.  Thanks.

[David Quon]

Hi Jens.  After looking at this some more I don't see this assertion when I dispatch_sync as described above.

        [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd

                                                            object:self

                                                              file:@(__FILE__)

                                                        lineNumber:__LINE__

                                                       description:

             @"***** THREAD-SAFETY VIOLATION: This database is being used on a thread it wasn't "

              " created on! Please see the concurrency guidelines in the Couchbase Lite "

              "documentation. *****"];

Looking at the backtrace above I'm thinking that the assert is happening due to the _cmd not being defined due to the block dispatch.  That would be consistent with the original error message given above.  I tried proving this today but for some reason I can't get Kiwi to work with a manually brought in CBL framework (without CocoaPods).  Do you have any advice you can lend me here?  Thanks.

[David Quon]

Actually I take that back.  Although I think the _cmd is what is actually causing the assertion it must be something with the threading that causes it to get to that assertion code due to this line.

ok = dispatch_get_current_queue() == dispatchQueue;

The only problem is the threading works fine on the device (non-testing) and seems to look okay looking at the backtrace.  Jens let me know if you have any other thoughts of what could be the problem.  Thank you.

[Jens Alfke]

On Apr 3, 2014, at 4:41 PM, David Quon <dav...@amcoonline.net> wrote:

Actually I take that back.  Although I think the _cmd is what is actually causing the assertion it must be something with the threading that causes it to get to that assertion code due to this line.

That’s a red herring. “_cmd” is a detail of the Objective-C runtime, an invisible parameter passed to a method whose value is the selector of the method. It’s being used after the assertion fails to generate the message for the exception. The fact that the assertion failed has nothing to do with _cmd.

The backtrace you sent me does seem to indicate that you’re calling onto a different dispatch queue, which would trigger that exception. The stack is the main thread, but in frame 16 you call dispatch_sync. The fact that this didn’t immediately deadlock indicates that the queue you’re dispatching to is not the main queue. (Remember, queues can run on different threads at different times.)

I don’t understand why you’re doing cross-queue dispatching from a method that’s already in your CBLModel subclass. If your model is being called, then you must already be running on the thread/queue that you use for Couchbase Lite, therefore there’s no need to jump to another one. Because if you were calling your model objects from a different thread/queue, that would already be a violation of the thread-safety rules.

—Jens

[David Quon]

Thanks Jens for the feedback.  You bring up some interesting points that I'll have to investigate further.

> Remember, queues can run on different threads at different times.

This is probably why I'm seeing different results between the device and Kiwi Unit testing.

>  If your model is being called, then you must already be running on the thread/queue that you use for Couchbase Lite, therefore there’s no need to jump to another one. Because if you were calling your model objects from a different thread/queue, that would already be a violation of the thread-safety rules.

The original problem was violations of the thread-safety rule without the dispatching.  Cross-queue dispatching seemed to fix this for me on the device but after your reasoning above it may not have been the correct solution.  I have some areas now to re-investigate.

Thanks for your informative feedback Jens.  It's greatly appreciated.

[David Quon]

Hi Jens.  After investigating this a bit more I'm still having some issue with this as I can't seem to see a distinct difference in thread or dispatch queue when running the app on the device (working) and running the app in the Kiwi Unit Tests (broken).  Are there any example apps that show how the background threading / dispatch queue code is supposed to work to see if I'm misunderstanding something?

If I'm understanding your code correctly then when the dispatchQueue is set in the CBLManager like this:

CBLManager.sharedInstance.dispatchQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);

The the check in FMDatabase in beginUse doesn't compare threads anymore but instead compares dispatch queues like this?

ok = dispatch_get_current_queue() == dispatchQueue;

Would this test fail if the thread that the queue is running on changes?  Would this test fail if a new instance of the queue was created even though it was created with the same name?  From the traces I'm doing it seems like the code is running on the correct dispatchQueue however obviously something is triggering the thread assertion in FMDatabase.  Thanks for your help as always.

[David Quon]

Out of curiosity if the dispatchQueue is set on CBLManager why is Couchbase Lite not just always running on that queue (as it's now required to do so otherwise an assertion will happen) regardless of the thread/queue of the calling entity?

[Jens Alfke]

On Apr 11, 2014, at 2:28 PM, David Quon <dav...@amcoonline.net> wrote:

If I'm understanding your code correctly then when the dispatchQueue is set in the CBLManager like this:

CBLManager.sharedInstance.dispatchQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);

Oh, that’s the problem right there — dispatch_get_global_queue returns a concurrent queue, a special kind that can run lots of blocks in parallel. You absolutely can’t use a queue like that with Couchbase Lite (or most other APIs) because it does absolutely nothing to prevent concurrent calls to the code.

Why are you trying to use this? I think you may have gotten it mixed up with dispatch_get_main_queue(). Usually the only time you use a concurrent queue is if you want a block to be run asynchronously and you don’t care where it’s run, and everything it does is already thread-safe.

Out of curiosity if the dispatchQueue is set on CBLManager why is Couchbase Lite not just always running on that queue (as it's now required to do so otherwise an assertion will happen) regardless of the thread/queue of the calling entity?

Because I didn’t want to wrap every single public method/function in a dispatch_async(…) or dispatch_sync(…) block. Not only would that be a big pain to add, it’s also tricky because once you’ve added dispatch_sync to a method you can no longer call it from any other method that does the same thing, or you deadlock. I’m not too familiar with this way of coding, but it looks like you’d have to have two layers of code — the public API does nothing but use dispatch_sync and call into the private API, which does all the work. And the private API can never call into the public API.

The purpose of the dispatchQueue and thread properties is not to make CBL thread-safe but to tell it what thread/queue to schedule async calls on.

—Jens

[David Quon]

> Oh, that’s the problem right there — dispatch_get_global_queue returns a concurrent queue, a special kind that can run lots of blocks in parallel.

That's right.  Maybe that is causing the inconsistency.  I was trying to create my own DISPATCH_QUEUE_SERIAL earlier but ran into issues and abandoned that.  I'll have to revisit this to see if it will fix the issue.

I'm just trying to get the CBL code to run on a defined queue or thread so the thread assertion won't happen anymore.  While I'd love to run on the main thread unfortunately some of the database communication is hanging the UI and thus the need to move off the main thread.  Since I don't want to effect the current flow of the existing code I'm using dispatch_sync.  Since we're using models created on the main thread it seems that all communication then needs to be moved off onto a database dispatch queue using dispatch_sync unless I'm misunderstanding something.

> Because I didn’t want to wrap every single public method/function in a dispatch_async(…) or dispatch_sync(…) block.

While I understand the complexity in doing this, isn't it the same as what the clients of CBL need to do if they want to run on a different dispatch queue or threads other than main now that CBL is checking and enforcing the thread or dispatch queue?  Seems like if it's mandatory to run on the same thread or dispatch queue once it's defined in the CBL manager that it would be better to have it enforced and handled by CBL.  I maybe misunderstanding something here though.

Are there any example projects that use models and dispatch queues?  Thanks for your help and feedback Jens.

[Jens Alfke]

On Apr 11, 2014, at 4:14 PM, David Quon <dav...@amcoonline.net> wrote:

That's right.  Maybe that is causing the inconsistency.  

Not maybe. It is definitely going to cause problems.

I was trying to create my own DISPATCH_QUEUE_SERIAL earlier but ran into issues and abandoned that.  I'll have to revisit this to see if it will fix the issue.

Just call dispatch_queue_create(“my-queue”, NULL);

Since we're using models created on the main thread it seems that all communication then needs to be moved off onto a database dispatch queue using dispatch_sync unless I'm misunderstanding something.

If you create any CBL object, including a model, on the main thread/queue, it has to be called on the main thread/queue.

If you want to move your CBL code onto a background queue, you have to move all of it, at least all of it that uses that set of objects. You can have one set of objects on the main thread and another set on the bg queue if you want.

Seems like if it's mandatory to run on the same thread or dispatch queue once it's defined in the CBL manager that it would be better to have it enforced and handled by CBL.

The general trend in concurrency design nowadays is to keep data local to a single thread (or queue) and use messages between threads, rather than trying to use locks to provide safe concurrent access to data. In my experience the latter is very difficult and can produce an endless series of increasingly subtle bugs. People have been burned by this enough that they’re rethinking it. A number of modern languages like Go, Rust, Erlang and Scala make this the primary type of concurrency. (I think I’ve also pointed out that Core Data has the same type of concurrency rules too. If you want multiple threads you have to create a separate MOC for each one.)

Anyway, as we are very very close to 1.0, this isn’t going to change soon. I do want to re-examine concurrency for the next major release and see if things can be made more flexible.

—Jens

[David Quon]

Thank you very much Jens again for your help and feedback.  I'll give the above a try to see if it helps to fix the problems I'm seeing.  Thanks again.