Don't force completion blocks to be scheduled on the main queue

[sharplet]

(This PR depends on #67.)

Different request performers are allowed to have different semantics when it comes to the queue that results are delivered on. If a performer already happens to deliver results on the main queue, APIClient is injecting latency by scheduling an unnecessary async call. If a performer delivers results on a background queue (i.e., NetworkRequestPerformer), and there's further processing that would benefit from occurring in the background (e.g., image processing or decoding a large object), callers must dispatch back onto a background queue to avoid blocking the UI. It also hampers composability of requests, because multiple chained requests will incur a penalty from jumping back and forth from the main queue to a background queue.

This breaking change removes the dispatc_async and leaves that decision to callers, as it's not possible to anticipate the correct decision to use in every situation.

[sidraval]

Yesssss yes yes, excellent find

[gfontenot]

The original reasoning for doing this is to mimic the existing behavior of popular libraries like AlamoFire. As a user, it drives me crazy to have to do this at the app level, especially since these kinds of requests tend to require UI updates 80% of the time. I think completely removing the behavior might not be the best idea, but I think injecting a specific queue to perform the completion block on might be interesting.

For example, something like:

diff --git a/Source/Models/APIClient.swift b/Source/Models/APIClient.swift
index 83bba08..52c75c4 100644
--- a/Source/Models/APIClient.swift
+++ b/Source/Models/APIClient.swift
@@ -11,10 +11,10 @@ public struct APIClient {
 }

 extension APIClient: Client {
-  public func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, T.ResponseError> -> Void) -> NSURLSessionDataTask {
+  public func performRequest<T: Request>(request: T, completionQueue: dispatch_queue_t = dispatch_get_main_queue(), completionHandler: Result<T.ResponseObject, T.ResponseError> -> Void) -> NSURLSessionDataTask {
     return requestPerformer.performRequest(request.build()) { result in
       let object = (result >>- deserialize >>- request.parse).mapError(request.transformError)
-      onMain { completionHandler(object) }
+      dispatch_async(completionQueue) { completionHandler(object) }
     }
   }
 }
diff --git a/Source/Protocols/Client.swift b/Source/Protocols/Client.swift
index c5df687..0935756 100644
--- a/Source/Protocols/Client.swift
+++ b/Source/Protocols/Client.swift
@@ -3,5 +3,5 @@ import Argo
 import Result

 public protocol Client {
-  func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, T.ResponseError> -> ()) -> NSURLSessionDataTask
+  func performRequest<T: Request>(request: T, completionQueue: dispatch_queue_t, completionHandler: Result<T.ResponseObject, T.ResponseError> -> ()) -> NSURLSessionDataTask
 }

This still performs a call to dispatch_async, which might still mean it introduces latency? it also clutters up the function declaration but that seems fine since it's fairly nice and clean at the call site.

I don't know that this necessarily fixes the issue, though. I'd be interested to know if this change (in the PR, not my diff) alone is enough to solve the performance issues @sidraval has seen in his client project.

[gfontenot]

Another thought: what's the actual issue here? Is the basic problem that chaining multiple requests results in introduced latency? If so, maybe there's a better way we can specify request chains or request dependencies? NSOperations have the concept of dependencies. Is this something we need/want?

[sharplet]

I think this summarises the issues for me:

• Chaining multiple requests involves unnecessary hops to the main thread and back, introducing latency.
• Forcing execution to change to a different queue reduces composability with other abstractions that handle scheduling, such as ReactiveCocoa.

I like the idea of being able to inject a queue. What do you think about making the queue optional, so if one is not specified the completion block will be executed synchronously? The main queue could potentially also be the default.

[gfontenot]

So you're suggesting something like:

diff --git a/Source/Models/APIClient.swift b/Source/Models/APIClient.swift
index 930a54c..093e2d4 100644
--- a/Source/Models/APIClient.swift
+++ b/Source/Models/APIClient.swift
@@ -11,10 +11,14 @@ public struct APIClient {
 }

 extension APIClient: Client {
-  public func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, SwishError> -> Void) -> NSURLSessionDataTask {
+  public func performRequest<T: Request>(request: T, queue: dispatch_queue_t? = dispatch_get_main_queue(), completionHandler: Result<T.ResponseObject, SwishError> -> Void) -> NSURLSessionDataTask {
     return requestPerformer.performRequest(request.build()) { result in
       let object = result >>- deserialize >>- request.parse
-      onMain { completionHandler(object) }
+      if let queue = queue {
+        dispatch_async(queue) { completionHandler(object) }
+      } else {
+        completionHandler(object)
+      }
     }
   }
 }
diff --git a/Source/Protocols/Client.swift b/Source/Protocols/Client.swift
index 9104b9c..6af3cf2 100644
--- a/Source/Protocols/Client.swift
+++ b/Source/Protocols/Client.swift
@@ -3,5 +3,5 @@ import Argo
 import Result

 public protocol Client {
-  func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, SwishError> -> ()) -> NSURLSessionDataTask
+  func performRequest<T: Request>(request: T, queue: dispatch_queue_t?, completionHandler: Result<T.ResponseObject, SwishError> -> ()) -> NSURLSessionDataTask
 }

[sharplet]

Yes, something like that. What do you think about a property on APIClient instead of a method parameter?

[gfontenot]

I guess the two arguments would be:

• I want to make sure all requests through this client perform their callback on the same thread
• I want to make sure I'm able to control the client performs the callback on on a request by request basis

So that means that if we assign the queue at the instance level, we'd have to create two clients to be able to satisfy the second argument. Conversely if we pass the queue in, there's a little bit of duplication at the call site when performing the request.

I'm honestly not sure which makes more sense, but I feel like I'm still leaning towards injecting the queue at the point where we perform the request.

[sharplet]

The only other issue I can think of with passing the queue in as a parameter to performRequest is that there's no way to specify a different default than the main queue. Or put differently, request callbacks will always be performed on the main queue, unless I remember to change every single callsite to pass in nil.

For that reason I think I'd prefer a parameter on APIClient. What do you think? This may just be a difference of opinion, however.

---

FWIW, I tried to see if we could allow both. Theoretically, that would look something like this:

public struct APIClient {
  var queue: dispatch_queue_t? = dispatch_get_main_queue()
}

extension APIClient: Client {
  public func performRequest<T: Request>(request: T, queue: dispatch_queue_t? = self.queue, completionHandler: Result<T.ResponseObject, SwishError> -> Void) -> NSURLSessionDataTask {
    // ...
  }
}

But alas, you can't access self in a parameter default.

[gfontenot]

Yeah, I think it's just a difference of opinion. Maybe the rest of @thoughtbot/ios can break the tie?

[tonyd256]

This is a tough call for me ... I can see what @sharplet wants and it makes sense; however, my 90% use case is that I want the call the main thread anyways so I like that swish does it for me. Would it be too much to have a second performRequest that did take a queue with a nil default?

What about a second completion block? performRequest(request:, completionOnMain:) and performRequest(request:, completion:)?

[sharplet]

@tonyd256 I think I'd prefer that to a single method call with a default parameter, because then they'll show up as two different autocomplete options in Xcode.

Although having said that, I'm now thinking that Xcode probably does the same thing for a single method with a default parameter!

An issue with having two versions that only differ by the label for the completion block is that when using trailing closure syntax it's now ambiguous which will be used (or maybe it just won't compile?).

[sharplet]

Regarding our difference of opinion: I've pushed 321aa79 which implements the Client-based queue configuration for your review. It may be interesting to compare this to a method-based implementation, so that we can get a handle on the differences in API complexity and usage.

[dazmuda]

Seriously good point about trailing closure syntax.

I am perfectly fine with the client based queue config since it doesn't require any config if you just want to default to the main queue.
And it accommodates your need to default to not the main queue.

So, my vote's for that.

[dazmuda]

Though, it would be nice to also have a method that takes a queue that takes first priority over the default queue.

Because it would be a pain to have to set the default queue property often.

So I want both, yes. 😁

[sharplet]

Also just pushed some tests for the queue handling stuff!

[sharplet]

Because it would be a pain to have to set the default queue property often.

In that case I'm thinking I'd probably instantiate a single APIClient and configure it once.

[gfontenot]

I don't think this actually tests anything. isOnMain is false by default, so this can't fail. Would probably be better to default it to true and remove the optionality.

[sharplet]

No, it's nil by default. The matcher will take care of the nil case, failing the test with a "use beNil() to match nils" message.

[gfontenot]

I don't think we should add multiple performRequest methods. That's adding way more complexity around this than we need.

I'm still not convinced that a property is the way to go here. I'm also a little worried that this is introducing complexity to the API based on speculation. Do we have any actual proof that this is an issue and that this is an actual fix? I thought I remembered that @sidraval said removing the queue stuff in the client app he was having performance issues in didn't result in any measurable performance improvement, but maybe I'm misremembering?

I'm very torn on this.

[sharplet]

I'm also a little worried that this is introducing complexity to the API based on speculation.

FWIW, if we close this PR I'll still consider it a successful discussion. As far as API design goes, that's exactly what this PR is: speculation! I think it's valuable to have something concrete to discuss.

So you raise a good point that so far my advocating for this hasn't been particularly concrete, and really my argument is more an appeal to principal. I personally believe that it's up to the app to decide whether dispatching everything onto the main queue is the correct default. I'm not convinced that it always is, and so I'm not convinced that Swish should force that policy on every app. In practice, perhaps it's super useful and the potential performance problems I'm citing are a non-issue. I'm totally happy if that's the case! I'd just feel more comfortable if this wasn't hard-coded.

[sidraval]

I'm in agreement with @sharplet here.

@gfontenot I did say that, though to be clear it was a 30m punt where I just quickly hacked it together to see if was the only problem.

However, reading the code made way more sense, because previously my RAC requests would wrapped in an onBackground { } and it looked like the callback was also getting called on the background as well. Removing the main thread default meant my RAC requests looked like:

(__request code__).observeOn(UIScheduler()).start {

which is more explicit, and clearer IMO.

[tonyd256]

Pipe Dream:

What if there was a Background Monad and we had a function runInBackground similar to runDB where it encapsulates a transaction. You can pass one request to it and it will perform that request and return to the main thread. You could also compose many requests or other background tasks and sequence them so that they all run in order before they are returned to the main thread. This would help with chaining tasks that need background threading but still allow a final return to the main thread.

Probably not easy but maybe gets the brain juices flowing for something.

[gfontenot]

Here's another option brought up by @jnutting that might actually be better than anything we've suggested so far:

diff --git a/Source/Models/APIClient.swift b/Source/Models/APIClient.swift
index 930a54c..f924e1d 100644
--- a/Source/Models/APIClient.swift
+++ b/Source/Models/APIClient.swift
@@ -11,10 +11,11 @@ public struct APIClient {
 }

 extension APIClient: Client {
-  public func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, SwishError> -> Void) -> NSURLSessionDataTask {
+  public func performRequest<T: Request>(request: T, scheduler: (() -> Void) -> Void = onMain, completionHandler: Result<T.ResponseObject, SwishError> -> Void) -> NSURLSessionDataTask {
     return requestPerformer.performRequest(request.build()) { result in
       let object = result >>- deserialize >>- request.parse
-      onMain { completionHandler(object) }
+
+      scheduler { completionHandler(object) }
     }
   }
 }
diff --git a/Source/Protocols/Client.swift b/Source/Protocols/Client.swift
index 9104b9c..f35d966 100644
--- a/Source/Protocols/Client.swift
+++ b/Source/Protocols/Client.swift
@@ -3,5 +3,5 @@ import Argo
 import Result

 public protocol Client {
-  func performRequest<T: Request>(request: T, completionHandler: Result<T.ResponseObject, SwishError> -> ()) -> NSURLSessionDataTask
+  func performRequest<T: Request>(request: T, scheduler: (() -> Void) -> Void, completionHandler: Result<T.ResponseObject, SwishError> -> ()) -> NSURLSessionDataTask
 }

So instead of passing in a queue, we just pass in a closure. And that closure can do whatever the hell it wants to do. It could just be { $0() }, or it could do some real wacky shit. It also removes the concern about the dispatch_async vs dispatch_sync bit. It should probably help with interaction with RAC schedulers as well, right? I'm not familiar enough with that lib to know for sure.

This seems way more flexible than anything else we've talked about.

There's still the question about where to pass it, but I'll concede to the group since it seems like people prefer passing it at the time of initialization. That would also make this a non-breaking change as well.

[sharplet]

I agree that the scheduler block seems like it may be the simplest change. What do you think about it having the type ((() -> Void) -> Void)? so that callers can opt for no scheduler (meaning invoke the block directly)?

[sharplet]

FWIW, I've pushed a version that uses the scheduler block, and rebased on master. Let me know what you think!

[gfontenot]

Do you think the optional is worth it here? It seems like a bit of magic, where consumers could very easily do

let directScheduler: (() -> Void) -> Void = { $0 }

and get the same behavior without adding complexity here or adding semantics to passing .None.

[gfontenot]

Only remaining question is about the use of the optional for the scheduler. I'm also curious about if you wanted to move the scheduler to the APIClient instance. I feel like you felt stronger about that than I did and was ready to back down.

[sharplet]

Ended up switching to the non-optional version of the scheduler, and introduced immediateScheduler and mainQueueScheduler, with the latter being the default. I also switched back to the property version, which I do prefer.

[sharplet]

Also I just pushed a second commit that shares the tvOS and watchOS schemes, which oddly weren't already shared. That should fix CI.

[gfontenot]

nice

[gfontenot]