Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is it possible to have default queue per Promise? #63

Closed
teerapap opened this issue Jul 31, 2018 · 4 comments
Closed

Is it possible to have default queue per Promise? #63

teerapap opened this issue Jul 31, 2018 · 4 comments

Comments

@teerapap
Copy link

From what I understand, the default dispatch queue is global and for all Promise created.

Is it possible to have the default queue per Promise?

Supplied when creating a Promise instance and be as a default queue for all then/always/catch/recover closures for this Promise unless specified otherwise for that specific closure.

The rationale is the global default affects too many things. Different Promise should run on queue with different QoS. Usually I have a Promise which should run as DispatchQueue. userInitiated and it chains many then/always/catch/recover closures down the line which I want it to run in the same queue by default.

With current implementation, I have to pass the queue to every closures. And at the outmost Promise, I have then/always/catch closure on DispatchQueue.main to update UI.

Thanks.

@shoumikhin
Copy link
Contributor

Hi Teerapap, that sounds like an interesting idea!

Do you suggest that we parametrize the Promise.pending (FBLPromise.pendingPromise) constructor with a custom queue arg having DispatchQueue.promises default value, store it within the promise and use in all convenience APIs that we currently have defaulting to DispatchQueue.promises? That way once the promise is created with a custom queue, everything you chain on it would be dispatched on that custom queue you've specified initially.

Although, you'd need to explicitly fall back to the main queue at some point of the chain to update UI, for example. Also, make sure other users of your promise are aware anything they chain on it will be dispatched on some custom queue.

Is that what you'd expect?

@teerapap
Copy link
Author

teerapap commented Jul 31, 2018

Hi shoumikhin,

Yes. Correct. Maybe explain with code is easier

Currently my code looks like this.

func callAPI(userId: String, on queue: DispatchQueue) -> Promise<Result> {
    return Promise<Result> (on: queue) {
        // call api
    }.then(on: queue) {
        return Result()
    }
}

func register(user: String, on queue: DispatchQueue) -> Promise<Result> {
    return Promise(on: queue) {
        // do something
        return db.getUserId(user)
    }.then(on: queue) { userId -> Promise<Result> in
        return callAPI(userId: userId, on: queue)
            .always(on: queue) {
                
            }
    }.recover(on: queue) { error in
        // do some error conversion
    } // more then closures
}

register("username", on: DispatchQueue.global(qos: .userInitiated))
.always(on: DispatchQueue.main) {
    // update UI on main queue
}.catch(on: DispatchQueue.main) {
    // show error UI on main queue
}.then(on: DispatchQueue.main) {
    // update UI  on main queue
}

It would be better if I don't have to pass the queue down the line like this

func callAPI(userId: String, on queue: DispatchQueue) -> Promise<Result> {
    return Promise<Result> (on: queue) {
       // ....
    }.then {
        // run in the same queue as above
        return Result()
    }
}

func register(user: String, on queue: DispatchQueue) -> Promise<Result> {
    return Promise(on: queue) {
        // do something
        return db.getUserId(user)
    }.then { userId -> Promise<Result> in
       // run on the same queue as above

        return callAPI(userId: userId, on: queue)
            .always {
                // run on the same queue as specified in callAPI(...)
            }
    }.recover { error in
         // run on the same queue as above
    }
}

register("username", on: DispatchQueue.global(qos: .userInitiated))
.always(on: DispatchQueue.main) {
    // update UI
}.catch(on: DispatchQueue.main) {
    // show error UI
}.then(on: DispatchQueue.main) {
    // update UI
}

@shoumikhin
Copy link
Contributor

Hi Teerapap,

The thing is setting a queue per promise and using it for the rest of the chain is slightly trickier than it may seem initially. That approach is potentially quite error prone, since you're gonna pass a promise around and chain other blocks on it without clearly knowing which queue it’s run on.

I'd recommend to be explicit in each block, or rely on the well defined default, especially if the chain spans multiple methods throughout a class, or maybe even across classes. Otherwise, I imagine, in future you'd end up having some asserts or checks in each closure to guarantee it's indeed dispatched on a specific queue and other collaborators didn't suddenly mess things up.

Also, such change may introduce some issues with the existing code:

promise.then(on: queue) { /*...*/ }.then { /*...*/ }

The second then above used to be dispatched on main by default, but with the proposed change it's gonna be dispatched on a background queue, which would be quite surprising for other clients.

Generally, it'd be very beneficial to follow a pattern when DB access, networking and other async APIs are always initiated from the main queue, but then internally use some dedicated background queues to do the real work. For example, you may have something like:

extension DispatchQueue {
  static let diskIO: DispatchQueue = { DispatchQueue.global() }()
  static let networkIO: DispatchQueue = { DispatchQueue.global() }()
  static let utility: DispatchQueue = { DispatchQueue.global() }()
}

func callAPI(userId: String) -> Promise<Result> {
  assert(Thread.isMainThread)
  return Promise<Result>(on: .networkIO) {
    // call API
  }  
}

func getUserId(_ user: String) -> Promise<UserId> {
  assert(Thread.isMainThread)
  return Promise<UserId>(on: .diskIO) {
    // fetch records from DB
  }  
}

func register(user: String) -> Promise<Result> {
  assert(Thread.isMainThread)
  db.getUserId(user).then(callAPI).always(on: .utility) {
    // ...
  }.recover(on: .utility) { error in
    // do some error conversion
  } // more then closures
}

register("username").always {
  // update UI on main queue
}.catch { error in
  // show error UI on main queue
}.then { value in
  // update UI  on main queue
}

Please let us know if this is unclear or if you have any additional questions.

Thank you!

@teerapap
Copy link
Author

teerapap commented Aug 7, 2018

Thanks for the explanation. I understand my suggestion will have issue with existing code.
and I understand your point of being explicit.

I don't think it will be error prone though because if I want to make sure a closure runs on specific queue, I would specify explicitly at the closure anyway to run on that queue.

The situation is sometimes I want to chain a closure that does not matter which queue it runs on. Ex.

.recover { error in 
// convert error to some new error
throw newError
}

For this case, I think running it on the queue the pending promise running on (ex. .userInitiated or whatever) is more suitable than relying on default (which is usually main queue and high priority).

Your approach with .utility, .networkIO, .diskIO queue is also acceptable.

Anyway, breaking the existing assumption is bad so please take it as a feedback for the next time.
Thanks

@teerapap teerapap closed this as completed Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants