google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.81k stars 295 forks source link

CancelToken for promise completion blocks #75

Closed virl closed 5 years ago

virl commented 6 years ago

Promises should have ability to remove registered fulfillment/rejection callbacks(blocks) WITHOUT influencing underlying operation that promise corresponds to.

Such CancelToken (often also called InvalidationToken) should also allow cancellation of callback scheduled on some queue from within the same queue, just in earlier block. In other words, cancellation of callback should work even if its block is already scheduled on the queue (but not yet started to execute).

shoumikhin commented 6 years ago

Hi Vladimir, is this a duplicate of #5 and #64?

virl commented 6 years ago

@shoumikhin It seems so, but availability of CancelToken is must-have anyway for promises to be usable in any UI code (for UITableView cells, for example), because iOS uses GC and we need reliable way to deallocate promise listeners. Otherwise common usage of promises by average programmer will lead to deadlocks and/or excessive memory consumption and/or race conditions.

shoumikhin commented 6 years ago

Yep, I know that's a long-awaited feature. The good news is once a promise gets resolved, all its listeners get scheduled on GCD, so the latter controls their lifespan further on. And currently, as we've figured out in #74, you can directly reject any promise you're willing to cancel beforehand with a special error and skip handling it in catch, for example. By the way, I believe that's approximately the way how it's gonna be implemented eventually (plus, an important feature of cancelling a parent promise whose listeners have all bailed early, and providing a corresponding callback in the work block itself). Meanwhile, you can also check out if a custom token would come handy by any chance.

virl commented 6 years ago

@shoumikhin That custom token cancels the underlying task, not the promise callbacks registration.

Just try to implement UITableView controller with your promises sdk, where each cell will asynchronously load its content from network or background database like Realm.

Each cell object can be reused by UITableView, so before cell promise resolves, cell can be used to display different content entirely.

Without CancelToken bundled in your SDK, it means that your SDK is not usable for most common of asynchronous UI loading use cases.

shoumikhin commented 6 years ago

Vladimir, I'm sorry for confusion. I do understand how important and convenient the cancellation story can be. I guess what I'm trying to say is that there's probably a way to achieve what you want, although maybe not the most elegant one, of course.

Say, for your example, the table view controller is probably the one initiating an async operation per each cell after dequeueing it from cache using some network layer, which apparently also caches responses. Such a network layer would have an API returning a promise containing data needed to setup the cell. The controller can become a delegate of each cell and be notified when a cell gets reused in order to tell the network layer to cancel a particular request, which, in turn, could use something like the custom token I referred in the previous reply. Also, if we can afford to disable prefetching, we can then use cellForRow(at:) inside the then callback chained to the network call inside dequeueReusableCell(withIdentifier:for:) to check against nil, and so learn if the cell with a particular indexPath has already been reused for free and skip the update.

Anyhow, I realize there can be more sophisticated use cases where the lack of proper cancellation may appear frustrating. So, thank you again for raising that concern!

virl commented 6 years ago

@shoumikhin

The controller can become a delegate of each cell and be notified when a cell gets reused in order to tell the network layer to cancel a particular request

No, it cannot, because telling the network layer to cancel the request doesn't mean that it will be cancelled — it can be fulfilled before the cancellation event is processed. And cancellation of underlying process may not be possible anyway in underlying API (like DB API).

Also, if we can afford to disable prefetching, we can then use cellForRow(at:) inside the then callback chained to the network call inside dequeueReusableCell(withIdentifier:for:) to check against nil

No, we cannot, because cell object can already be reused for different indexPath entirely.

shoumikhin commented 6 years ago

No, it cannot, because telling the network layer to cancel the request doesn't mean that it will be cancelled — it can be fulfilled before the cancellation event is processed. And cancellation of underlying process may not be possible anyway in underlying API (like DB API).

How would cancelling a promise be helpful in that situation, if it's been already fulfilled with the response and then block is already scheduled on GCD? As I understand, a promise can't change its state after it's been resolved, even if cancellation is requested. So we still need to find a way to check if the cell has been reused inside the then block, even if we tried to cancel the promise somewhere at prepareForReuse moment.

No, we cannot, because cell object can already be reused for different indexPath entirely.

Given prefetching is turned off, I basically meant something like:

 func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
  guard let cell = collectionView.dequeueReusableCell( // ...
  cell.text = "hello world"
  // ...
  client.fetchImage(at: photoURLs[indexPath]).then { [weak collectionView] image in
    if let cell = collectionView?.cellForItem(at: indexPath) as? ImageCell {
      cell.image = image
    }
  }
  return cell
}
virl commented 6 years ago

@shoumikhin

How would cancelling a promise be helpful in that situation, if it's been already fulfilled with the response and then block is already scheduled on GCD?

CancelToken in that UI case is used for cancelling not the promise, but its observation callback.

So we still need to find a way to check if the cell has been reused inside the then block, even if we tried to cancel the promise somewhere at prepareForReuse moment

No, upon cell reuse we just cancel the CancelToken supplied when registering observation listener in promise. For new promise we just register new observation listener with new CancelToken.

That way entire cell reuse usecase is handled via single CancelToken field inside cell object.

Given prefetching is turned off, I basically meant something like:

That code is not usable in any performant app (and that's kind of app where async promises are needed in the first place), because cellForItem() works only for visible cells. But we need to start loading content before cells appear on the screen.

shoumikhin commented 6 years ago

CancelToken in that UI case is used for cancelling not the promise, but its observation callback.

Sorry, not sure I get that. How do you cancel an observation callback w/o cancelling the promise? The callback is something which is invoked passively based on the promise state changes. We can't fulfill a promise, and meanwhile skip invoking its listeners' success callbacks.

No, upon cell reuse we just cancel the CancelToken supplied when registering observation listener in promise. For new promise we just register new observation listener with new CancelToken.

Would be awesome to see a pseudocode example, if you don't mind.

That way entire cell reuse use case is handled via single CancelToken field inside cell object.

I have some concerns regarding having such a cancellation token stored in a cell. Specifically, that may potentially break MVC badly.

But we need to start loading content before cells appear on the screen.

Yup, exactly why I mentioned the prefetching clause.

virl commented 6 years ago

@shoumikhin

How do you cancel an observation callback w/o cancelling the promise?

Easily — with object called CancelToken that is checked on the callback's queue by the Promise before calling listener callback. If it is cancelled, callback will not be called.

Also CancelToken itself have .cancelledPromise field, which triggers when token is cancelled. Via that field the Promise can register listener to unregister user's listener when supplied cancel token is cancelled.

It is important because iOS doesn't have GC and we need to provide the way to cancel Promise listener callback with its full deallocation/release.

The callback is something which is invoked passively based on the promise state changes. We can't fulfill a promise, and meanwhile skip invoking its listeners' success callbacks.

We can. CancelToken is exactly for that, altought itself it should not be bound to only this use case.

In BrightFutures and similar frameworks it is often called InvalidationToken.

virl commented 6 years ago

@shoumikhin

MyCell : UITableViewCell
{
    var image: UIImage? = nil
    var cancelTokenSource = CancelTokenSource()

    func onCellReuse(_ contentId: String)
    {
        self.cancelTokenSource.cancel()
        self.cancelTokenSource = CancelTokenSource()

        let ltoken = self.cancelTokenSource.token

        self.image = nil

        network.loadContent(contentId) { [weak self] content in
            guard let sself = self else {
                return
            }

            guard !ltoken.isCancelled else {
                return
            }

            sself.image = content
        }

    }
}
MyCell : UITableViewCell
{
    var image: UIImage? = nil
    var cancelTokenSource = CancelTokenSource()

    func onCellReuse(_ contentId: String)
    {
        self.cancelTokenSource.cancel()
        self.cancelTokenSource = CancelTokenSource()

        let contentPromise = network.loadContent(contentId)

        self.image = nil

        contentPromise.onFulfill(self.cancelTokenSource.token) { [weak self] value in
            guard let sself = self else {
                return
            }

            sself.image = value
        }
    }
}
shoumikhin commented 6 years ago

Great, thank you for the examples!

Although, I have a few concerns now. Looks like MyCell, being a view, plays a role of a controller. It keeps refs to model objects and has an important chunk of business logic for updating them. Hope, that's made for brevity only, and in reality that piece is implemented in a controller which owns the table view, or elsewhere. So, let's ignore that.

Easily — with object called CancelToken that is checked on the callback's queue by the Promise before calling listener callback. If it is cancelled, callback will not be called.

That's an interesting idea. By looking at the example, I imagine a promise could just provide a cancel() method with the same success (since CancelTokenSource likely has a ref to the promise) to set that cancelledPromise kept internally and check it before invoking success callbacks.

Anyhow, I'm not convinced yet we should cancel already scheduled success callbacks and disregard already sealed promise's state just because the cancellation was requested after the promise was fulfilled, but before the callbacks had a chance to execute. Sounds like part of the callbacks can potentially be already executed, and the rest got skipped because of such cancellation, whereas the promise is still considered fulfilled when tested explicitly, which may lead to tricky unpredictable errors. A promise must be either rejected with cancellation reason, or finally fulfilled and notify everyone about that. If it's already fulfilled, we can't just retain it from invoking callbacks.

It seems safer to simply pass a custom token inside the block and check if it's set there instead. Which is essentially what you propose, I guess, but without any promises-related APIs.

It is important because iOS doesn't have GC and we need to provide the way to cancel Promise listener callback with its full deallocation/release.

In our implementation a promise schedules all callbacks on GCD (thus, passes the ownership) or deallocates them once it's resolved. And cancellation means resolution.

virl commented 6 years ago

By looking at the example, I imagine a promise could just provide a cancel() method with the same success (since CancelTokenSource likely has a ref to the promise)

No, CancelTokenSource doesn’t have ref to promise. No, promise cannot provide cancel() method, because we need to cancel (unregister) specific callback, not promise.

cancelledPromise is entirely different promise instance that is needed to receive async notifications about cancel token cancellations.

Anyhow, I'm not convinced yet we should cancel already scheduled success callbacks and disregard already sealed promise's state just because the cancellation was requested after the promise was fulfilled, but before the callbacks had a chance to execute.

Why? I’ve registered listener on Promise. Now I want to unregister it without waiting for promise completion.

How can I do that in your library?

Sounds like part of the callbacks can potentially be already executed, and the rest got skipped because of such cancellation, whereas the promise is still considered fulfilled when tested explicitly, which may lead to tricky unpredictable errors.

No, it is completely normal, because each callback has its own owner that manages it and knows when it cancels it and for what.

If it's already fulfilled, we can't just retain it from invoking callbacks.

You’re not retaining it. You’re unregistering callbacks from it.

Just like you would do .delegate = nil in iOS or unregisterListener() in Android.

It seems safer to simply pass a custom token inside the block and check if it's set there instead. Which is essentially what you propose, I guess, but without any promises-related APIs.

Yes, but that way the block itself will not be deallocated upon token cancellation and all captured block variables will remain retained until promise completes.

And cancellation means resolution.

No, cancellation means unregistering observation of promise state.

shoumikhin commented 6 years ago

No, CancelTokenSource doesn’t have ref to promise.

Gotcha, the promise just checks token's state synchronously. Well, a custom token would serve a bit better, since we could skip synchronization and assume it's always accessed on the same thread. But it's not that important, of course.

I’ve registered listener on Promise. Now I want to unregister it without waiting for promise completion.

The thing is by "registering a callback" on a promise you actually create another promise chained on it, and that others can chain onto later, which is waiting for the original promise to resolve. So, technically you can't "unregister a callback", but you can resolve the root promise and influence the state of all chained ones, i.e. which callbacks (if any) will be invoked as the result of such resolution. Or resolve any chained promise directly, of course. That's why I said you can reject the chained promise (influence its state) and so skip executing the success block entirely. Just be sure to do that before that promise got fulfilled. Otherwise, there's no way to change its state anymore and the block will be executed. Or just introduce a custom flag to check it in the success block and bail early. That's even more flexible, IMO.

How can I do that in your library?

For the example above, you do approximately the following:

func onCellReuse() {
  promise.reject(/* predefined erro for cancellation */)
  promise = network.loadContent(...).then { value in 
    // do the work...
  }
}

Which will try to avoid doing the work, unless the content has already loaded. Or you go the token way, i.e.:

func onCellReuse() {
  cancelToken.cancel()
  cancelToken = CancelToken()
  network.loadContent(...).then { [cancelToken] value in 
    if (cancelToken.isCancelled) { return }
    // do the work...
  }
}

Yes, but that way the block itself will not be deallocated upon token cancellation and all captured block variables will remain retained until promise completes.

In case a promise was fulfilled, all error callbacks get deallocated immediately. And vice versa. All success callbacks get scheduled on GCD and so are solely owned by it until executed and deallocated.

virl commented 6 years ago

@shoumikhin Ah, you need support for promise chaining. Then yes, you're right — promise in your lib cannot use CancelToken directly.

shoumikhin commented 6 years ago

Hi Vladimir, If I'm not mistaken, you suggested to have something similar to InvalidationToken?

Its implementation seems pretty trivial and it has nothing to do with promise chaining. It also never breaks the premise that a promise cannot change its state after it's been resolved.

The only convenient part there is that you get an early return for free.

With the same success you could implement a simple InvalidationToken like so:

class InvalidationToken {
  public var isValid: Bool {
    pthread_mutex_lock(&mutex)
    let isValid = valid
    pthread_mutex_unlock(&mutex)
    return isValid
  }

  public func invalidate() {
    pthread_mutex_lock(&mutex)
    valid = false
    pthread_mutex_unlock(&mutex)
  }

  private var valid = true  

  private var mutex: pthread_mutex_t = {
    var mutex = pthread_mutex_t()
    pthread_mutex_init(&mutex, nil)
    return mutex
  }()
}

And rewrite the example using Google Promises as follows:

class MyCell : UICollectionViewCell {
  var token = InvalidationToken()

  public override func prepareForReuse() {
    super.prepareForReuse()
    token.invalidate()
    token = InvalidationToken()
  }

  public func setModel(model: Model) {
    let token = self.token
    ImageLoader.loadImage(model.image).then { [weak self] UIImage in
      if (token.isValid) {
        self?.imageView.image = UIImage
      }
    }
  }
}

(again, ignoring the error-prone design when a cell performs functions of a controller)

As you see, it doesn't seem like too much work, and can be implemented using pretty much any promises library out there.

You can even argue having an explicit isValid check inside the success callback may look cleaner for people not familiar with specific promises APIs and also come handy when you want to do something before an early return, e.g. log the cancelled event first, etc.

virl commented 6 years ago

@shoumikhin Yes, I know. But thanks for the example anyway.

ghost commented 5 years ago

Thank you for the discussion. Please feel free to reopen if there are anymore questions/comments regarding this topic.