haxetink / tink_core

Core utilities
https://haxetink.github.io/tink_core
MIT License
118 stars 33 forks source link

Cancel loser #140

Closed kevinresol closed 4 years ago

kevinresol commented 4 years ago

This allows cancelling the loser of first()

But I wonder should it be true by default, but that would be breaking.

If that is desired I can add a compete() function and deprecate first()

kevinresol commented 4 years ago

My use case is that I use first() to timeout some operation that could possibly never resolves:

Future.delay(1000, Failure(new Error('Timeout'))).first(someOperationThatCouldNeverResolve()).handle(...)

In that case when the timeout is fired I would like to cancel the actual operation.

In a simple handle() case I can cancel the callback link. But non-trivial cases such as Promise.retry it seems very difficult to achieve so.

back2dos commented 4 years ago

Oh, weird. This was supposed to work already, but I must admit I didn't check. The SuspendableFuture should really dissolve its link when it's triggered. Then the desired behavior would fall out for free.

I think something like this should work:

  function trigger(value:T)
    switch status {
      case Ready(_):
      default:
        status = Ready(value);
        var link = this.link;
        this.link = null;
        wakeup = null;
        callbacks.invoke(value);
        link.cancel();
    }

But it's better to test ^^

kevinresol commented 4 years ago

Updated per your comment.

But I am not very sure if the first future should be cancelled too:

asserts.assert(cancelled1);
back2dos commented 4 years ago

Hmm, well, it's not so much about first vs. second. My suggestion was to clean up any future after it completes.

In some cases it makes sense:

var nextClick = new Future(yield -> {
  function onClick(e:MouseEvent) yield(e);
  document.addEventListener('click', yield);
  return document.removeEventListener.bind('click', yield);
});

In other cases, it may not have the desired effect:

function loadImage(url)
  return new Promise((resolve, reject) -> {
    var img = document.createImageElement();
    img.src = url;
    img.onerror = function () reject(new Error('failed to load $url'));
    img.onload = function () resolve(img);
    return function () img.src = '';
  });

The last line will have to be if (img.naturalWidth == 0) img.src = ''.

So it's fair to summarize saying that it's good at preventing leaks (which tend to be harder to observe / locate) at the cost of being "overly destructive" (the effect of which should be immediately obvious). I'm inclined to say it's a good trade-off (although it will require proper documentation).

What do you think?

kevinresol commented 4 years ago

Yeah that's my concern too. I think it is very fine if this is clearly specified.

kevinresol commented 4 years ago

Let's merge this first (as I want to fix the tests for nightly for the transitive abstract cast thingy)