Potential usability improvement - reducing or logging deadlocks

[jsutwanttohelp]

Yapdatabase can be accidently deadlocked by executing a synchronous read block within a synchronous read block, or a read block within a synchronous read/write block, or a read/write block within a synchronous read/write block.

As an example readWithBlock currently looks like this:

- (void)readWithBlock:(void (^)(YapDatabaseReadTransaction *))block
{
        ...
    dispatch_sync(connectionQueue, ^{ @autoreleasepool {
      /// run the block 
    }});
}

Perhaps it could look like this:

- (void)readWithBlock:(void (^)(YapDatabaseReadTransaction *))block
{
        ...
        if (dispatch_get_current_queue() == connectionQueue) {
      /// run the block 
         return;
        }
    dispatch_sync(connectionQueue, ^{ @autoreleasepool {
      /// run the block 
    }});
}

I haven't fully thought this through to see if there are any downsides.
For example I haven't thought about how yapdatabase would prevent this type of logic from incorrectly promoting a read block to a read write block.
Perhaps as an alternative yapdatabase should log that the deadlock is about to happen.

[cdzombak]

dispatch_get_current_queue is deprecated because, among other reasons, usages like this proposal, while they may appear correct at first glance, can easily still deadlock.

/*!
 * @function dispatch_get_current_queue
 *
 * @abstract
 * Returns the queue on which the currently executing block is running.
 *
 * @discussion
 * Returns the queue on which the currently executing block is running.
 *
 * When dispatch_get_current_queue() is called outside of the context of a
 * submitted block, it will return the default concurrent queue.
 *
 * Recommended for debugging and logging purposes only:
 * The code must not make any assumptions about the queue returned, unless it
 * is one of the global queues or a queue the code has itself created.
 * The code must not assume that synchronous execution onto a queue is safe
 * from deadlock if that queue is not the one returned by
 * dispatch_get_current_queue().
 *
 * When dispatch_get_current_queue() is called on the main thread, it may
 * or may not return the same value as dispatch_get_main_queue(). Comparing
 * the two is not a valid way to test whether code is executing on the
 * main thread.
 *
 * This function is deprecated and will be removed in a future release.
 *
 * @result
 * Returns the current queue.
 */
__OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_6,__MAC_10_9,__IPHONE_4_0,__IPHONE_6_0)
DISPATCH_EXPORT DISPATCH_PURE DISPATCH_WARN_RESULT DISPATCH_NOTHROW
dispatch_queue_t
dispatch_get_current_queue(void);

(dispatch/queue.h, iOS 9.2)

For additional information:

• 6. Re: dispatch_get_current_queue() Deprecated goes into the reasons for this deprecation
• Alternatives to dispatch_get_current_queue() for completion blocks in iOS 6?
• Why did Apple deprecate dispatch_get_current_queue?

This issue should be closed.

[cdzombak]

This issue should be closed.

At most, IMO the only change that should be made could be some warnings in the header documentation advising against nesting read/write blocks.

[jsutwanttohelp]

I agree that using dispatch_get_current_queue isn't the solution.

But silently deadlocking doesn't seem very helpful either.

Also, if YapDatabase supported recursive blocks it would significantly improve its usability.

As is, I need to write a wrapper method for anything that initiates a read or write to check if it is already within a transaction:

-(void)readSomething:(YapDatabaseReadTransaction*)transaction {
    if (transaction) {
        [self readSomethingHelper:transaction];
    } else {
        [[self getConnection] readWithBlock:^(YapDatabaseReadTransaction *transaction) {
            [self readSomethingHelper:transaction];
       }];
    }
}
-(void)readSomethingHelper:(YapDatabaseReadTransaction*)transaction {
    // Really do the read
}

[JimRoepcke]

Transaction blocks should just do reading and writing, and get out. Don't call other methods that are going to then call other methods that could eventually start another read or write. These problems are indicative of insufficient layering / architecture within an app. The rules are simple to follow and I've used Yap for years without this being a problem.

[JimRoepcke]

Another thing that helps is using the YapDatabaseModifiedNotification. If you write something, instead of writing it in a read/write transaction and then sending it to somewhere else, have that somewhere else observe the notification and receive the changes outside of the transaction.

[jsutwanttohelp]

I am sure it is possible, but reusable functions are not necessarily indicative of insufficient architecture or layering. However that is not the point of this issue.

Let me try to reframe this issue: Would YapDatabase be easier for developers to use if it reported or avoided deadlocks when that is possible?

[robbiehanson]

I discuss this deadlocking issue in the "Thread Safety" wiki page:
https://github.com/yapstudios/YapDatabase/wiki/Thread-Safety#connections-queues--deadlock

Yapdatabase can be accidently deadlocked by executing a [transaction within a transaction]

Yes, this is true. And you can also deadlock by executing a dispatch_sync within a dispatch_sync (on the same queue). And you can also deadlock by attempting to lock a locked lock (while already holding the lock).

In other words, there are a lot of threading technologies that don't prevent you from shooting yourself in the foot. But you do bring up an interesting question that I'll get to momentarily.

As is, I need to write a wrapper method for anything that initiates a read or write to check if it is already within a transaction

By allowing your transaction parameter to be nil, you're allowing this method to accidentally be called from within another transaction (by simply passing a nil parameter), and thus opening up the potential for deadlock. Simply change your code to look like this:

-(void)readSomething:(YapDatabaseReadTransaction*)transaction {
    NSAssert(transaction != nil, @"transaction parameter is required");
    // Really do the read
}

I've been there: when you first start writing these small utility methods, it seems convenient and easy to simply allow them to create their own transaction. But this is a mistake. Not only are you increasing the chances for deadlock, but by hiding little transactions all over the place, you're increasing the chance that your code-path will actually require multiple transactions to accomplish something. For example, imagine you have a couple dozen of these types of methods scattered throughout the codebase. Now fast-forward through several months of development, and imagine a code-path that ends up calling several of these types of methods. It would have been far more efficient to create one transaction, and then invoke each method (and pass the transaction parameter), thereby recycling a single transaction. Additionally, you get the benefit of atomicity. That is, each utility method sees the same transaction, and thus sees the exact same commit. Goofy things can happen when utility methods see different commits, but their results are combined together.

Would YapDatabase be easier for developers to use if it reported or avoided deadlocks when that is possible?

In a word: yes

I'm leaving this issue open while I investigate this.

[robbiehanson]

Added deadlock detection. Should throw an assert if detected. Let me know what you think.

[jsutwanttohelp]

This looks like a good change to me.

I think that the string in the NSAssert may be confusing to users.
Perhaps it would be helpful to include the link to the documentation explaining the issue in that string: https://github.com/yapstudios/YapDatabase/wiki/Thread-Safety#connections-queues--deadlock