method callbacks should be called on main queue by default or configurable queue

[alexcorre]

Right now METMethodInvocation executes both _receivedResultHandler and _completionHandler. On the queue associated with the METMethodInvocationCoordinators NSOperationQueue.

This isn't a bug because you dont make any guarantees that callbacks be called on the main thread. But I'm going to suggest that this is much clearer to the users of the lib. API when calling methods in METDDPClient currently looks like this:

[self.ddpClient logoutWithCompletionHandler:^(NSError *error) {
      // WE'RE ON SOME BACKGROUND QUEUE HERE       
}];

This forces the user to dispatch back to main queue where in fact I believe that will be the most common use case.

What do you think @martijnwalraven ?

[alexcorre]

Also if you have any suggestions on where to start in a pull request for this let me know as I'd love to contribute back.

I started working here to dispatch_sync back to main queue there, but this breaks the unit tests as they execute this method directly on the main thread and thus dispatching sync would cause deadlock.

Also dispatch_async to main queue here breaks unit tests as it can't verify that that handler is invoked after data updates are flushed. See this test;

This was just a test attempt to see if I could make a quick fix. Ideally the queue on which to execute the block should be configurable, or default to the queue that called the method.

[alexcorre]

Same goes for global NSNotifications that are exposed in METDDPClient

[martijnwalraven]

I completely agree. In most cases, being called back on the main queue is what you want so this should be the default to avoid a common source of bugs. Ideally, the queue would indeed be configurable. As far as I know, there is no reliable way to default to the queue that called a method (dispatch_get_current_queue is deprecated). We could add a queue: argument to all methods that take a callback block and that defaults to the main queue (but that wouldn't work for notifications) or specify this at a more global level. A callbackQueue property on METDDPClient is probably sufficient for most uses (other name suggestions welcome).

I'd have to look more into this to be sure, but maybeInvokeCompletionHandler seems like the right place to dispatch to a different queue. That might require changes to the unit tests. In other places, I use XCTestExpectation/waitForExpectationsWithTimeout: to test async code. Here the issue seems to be that I wanted to test that the callback isn't invoked when only one of didReceiveResult:error and didFlushUpdates: is called. But I don't like the dependency on synchronous invocation and would prefer to find another way to test this.

If you have any ideas about this, please try them out and let me know or make a pull request. I probably won't have time to work on this myself before next week.