Promise Cancellable Testing

[CMCDragonkai]

Description

The PromiseCancellable requires testing in cancellation behaviour and its behaviour in DAGs

Issues Fixed

• Related Asynchronous Promise Cancellation with Cancellable Promises, AbortController and Generic Timer Polykey#297

Tasks

• 1. Add tests for all instance and static methods
• 2. Change to using the constructor as the main location for handling the default signal

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[CMCDragonkai]

Still need to do DAG rejection where we want to test local and global side effects.

One of the trickiest parts is where there's an intermediate promise. In that area, early rejection can be a problem, because P3 is already rejected.

One way to avoid early rejection with P1, P2, and P3 chain. Is to pass a signal handler that doesn't do anything. This ends up causing P3 cancellation to wait on P1, and then leave it to P2 to decide what to do.

The alternative is to put a catch on P2 and handle any rejections that occur there with the understanding that P3 may already be rejected.

[CMCDragonkai]

I've tested with normal promises and in fact this is the default behaviour of promises if rejection were to occur early:

import process from 'process';

process.on('unhandledRejection', (r, p) => {
  // @ts-ignore
  console.log('UNHANDLED', r.name, r.message);
});

class CustomPromise<T> extends Promise<T> {

  public reject;

  constructor(executor) {
    let reject_;
    super((resolve, reject) => {
      reject_ = reject;
      executor(resolve, reject);
    });
    this.reject = reject_;
  }

}

async function main () {

  const p1 = new CustomPromise<string>((resolve) => {
    setTimeout(() => {
      resolve('p1 result');
    }, 100)
  });

  const p3 = p1.then(() => {
    const p2 = new CustomPromise<string>((resolve, reject) => {
      setTimeout(() => {
        reject(new Error('P2 REJECT'));
      }, 100);
    });
    return p2;
  });

  const p5 = p3.then(() => {
    return new CustomPromise<string>((resolve) => {
      resolve('lol');
    });
  }, (r) => {
    console.log('P4 is never produced');
    throw r;
  });

  // @ts-ignore
  p3.reject(new Error('P3 REJECT'));

  try {
    const r = await p5;
    console.log(r);
  } catch (e) {
    console.log(e.name, e.message);
  }

}

void main();

The result looks like:

P4 is never produced
Error P3 REJECT
UNHANDLED Error P2 REJECT

Therefore, we can say that a promise DAG can be:

P3 (P1 -> P2)

Where there are "nested" relationships. The arrow here represents a function computation. That is P2 doesn't exist until P1 has resolved. Therefore early rejection of P3 leaves P2 dangling as it is now an unhandled promise rejection.

Also consider this:

const p5 = p1.then(() => p2).then(() => p4)

There there are implicit promises between the then operations, but since the syntax is pointfree style, we can say that:

P1 -> P2 -> P4

Such that P3 only exists when P2 resolves, and P2 exists only when P1 resolves.

This also means though, you can't directly cancel P3, because it is nested.

The true dag is more like:

P5(P4(P1 -> P2) -> P3)

If P2 and P4 were to exist outside, and only brought in internally, one would argue that their side effects are already in-flight.

[CMCDragonkai]

It does appear that P3(P1 -> P2) means that cancelling P3 if intended to chain, should actually be cancelling P1. Not P2, because P2 doesn't exist yet.

Now if P1 is already done by the time P3 is cancelling, then the cancel signal would be ignored by P1 because double rejection is a noop, then the computation to P2 can be informed to also reject.

If we want that the default rejection not to cause problems, it seems that we would want:

p3 = p1.then(() => p2, reject?);

To be desugared to:

p1.then(() => p2).catch(reject?)

So that it is possible for p2 rejection to occur and just be caught...

Let me try this..

[CMCDragonkai]

It actually works. Simply by sticking catch(r => throw r) after the then, it ends up handling the rejection and converting it to an exception.

In fact because the default handler is just to throw the reason. It's simple enough to just do catch().

So:

const p3 = p1.then(() => p2).catch();

Is enough to allow p3 to do an early rejection without being affected by an unhandled promise rejection in p2.

This does not work if instead we instead did:

const p3 = p1.then(() => p2.catch());

Part of the reason is that catch itself produces a new promise so that p3 is really rejecting the return of the catch, and not the return of the then. It appears by doing this, there's no more unhandled promise rejection, allowing the p2 function to also reject if it wants to.

[CMCDragonkai]

We will need to test this for all the other binders like catch itself.

If catch itself returned a promise, that also gets aborted, we would want to prevent any kind of unhandled promise rejection there.

At the same time we may have to change to using the species feature so that super refers to the native Promise, so I can avoid too many intermediate useless AbortController objects.

[CMCDragonkai]

I have found I don't need the catch() trick anymore. This is because by using PromiseCancellable.from, it ends up adding a handler to any rejection on the intermediate p2 promise for both then and catch and there are no more unhandled promise rejections.

[CMCDragonkai]

Furthermore I have found the species is required but it doesn't actually do what I thought it did. It turns out super.then() now returns Promise, while super.catch() actually returns PromiseCancellable. This is pretty strange.

[CMCDragonkai]

In fact, it doesn't change the fact that PromiseCancellable.all still returns an instance of PromiseCancellable. So I'm actually not sure what the species is really doing then.

[CMCDragonkai]

After checking some issues for the @@species, it seems the docs isn't comprehensive on how it works. It doesn't actually affect alot of static calls.

See: tc39/ecma262#151 (comment)

What's interesting is that then still uses it, but catch doesn't... at least in my tests.

[CMCDragonkai]

Ok, the reason why super.catch and super.finally appear to produce PromiseCancellable already, is because they both actually end up calling this.then.

So because this.then already produces a PromiseCancellable, the result of catch and finally also produce PromiseCancellable.

So now I'm checking if I can optimise this so it's not producing an additional PromiseCancellable for no reason.

One thing that is challenging is the fact that because then is already called, and the default behaviour is connect its signal to cancellation, it appears that we should actually replace the abortController when catch or finally is called.

[CMCDragonkai]

So basically all I have to do is to mutate the promise returned by this.then and then proceed to use it. The tests continue to pass. The hardest to debug is if there are invisible side effects. Would be ideal to use a debugger here.

[CMCDragonkai]

It appears that the PromiseCancellable.all does end up using PromiseCancellable.then, however it doesn't actually return the result here. Instead a new promise is constructed for this, thus the from isn't being used.

I can imagine something like:

all(p) {
  return new PromiseCancellable((resolve, reject) => {
    p.then((v) => { ... });
  });
}

This would explain why the default cancellation doesn't do anything.

Because we cannot associate the signal to anything here, we would have to add the default functionality here.

Now we could use call PromiseCancellable.from here to reassociate the default handler. But again this seems like a waste of resources, we already have a PromiseCancellable, it's just a raw one, and not one produced by then.

This seems kind of flaky unfortunately. Perhaps this would have worked better if the constructor itself already associate the default signal handler. I just didn't do that originally thinking the signal handler is already custom as part of the executor. But this is making me rethink things.

[CMCDragonkai]

The reason it is flaky is that we won't really know which of the static methods vs instance methods are going to be using new vs then or some other method. They could be implemented in a variety of ways.

Now that then implies from, this means that if it is using new, we have to associate the default signal handler, but if it is using then, we do not, because it is already associated.

So far it seems like all the static methods will be using the new method, whereas all the instance methods EXCEPT then itself will be using then.

So we could fix this right now by associating the default signal handler for all static methods, while not doing so for catch and finally.

Alternatively IF the default signal handler was done in the constructor. Then if the symbol species was removed, we would end up with all static methods having the signal handler properly done, and all instance methods rely on then, which itself ends up using the constructor as well and having its default signal handler associated too.

Then each method just needs to override the signal handler if a custom signal handler is passed in, or override the abort controller if it is passed in.

[CMCDragonkai]

We could reserve the usage of onabort for the new constructor, so that the new usage can still override the signal handler by doing signal.onabort = null or attaching it on onabort... Either way this is an additional statement to "disable" the default early rejection.

We could also make the new PromiseCancellable take the PromiseCancellableController, such that if it is undefined, it can default on the default handler.

This still works...

const pC = PromiseCancellable(executor, (signal) => {
  console.log(pC);
});

Although if you are supplying a custom signal handler, it's quite likely you do not want to do early rejection of the pC.

However how would one do both early rejection AND something else. I guess that's where you can use both the executor and the signal handler.

new PromiseCancellable((resolve, reject, signal) => {
  signal.onabort = () => {
    reject(signal.reason);
    // do other stuff
  };
});

[CMCDragonkai]

Actually I found that I couldn't do this cleanly because of the case where a PromiseCancellable is simply constructed around something taking the signal. It would only work if we did signal.onabort = null; but this just looks wrong.

On the other hand, the current way with the fix works, but perhaps only for nodejs, if other implementations of promises worked differently this could cause a problem. I'm not sure, will require testing later in the browser to see.

[CMCDragonkai]

Another idea, we put a Proxy around the AbortSignal. This way we can can intercept the addEventListener function or onabort property.

If it is set, it takes precedence. If it is not set, we will default to early rejection. This way early rejection is set by default. One would argue that if you really want a PromiseCancellable where the cancel does nothing, just add an empty handler to the signal.

This then unifies it under the constructor, and everything relies on the constructor now.

[CMCDragonkai]

Here's an example of using proxies:

const abortController = new AbortController();

let signalHandled = false;
const signal = new Proxy(abortController.signal, {
  get(target, prop, receiver) {
    if (prop === 'addEventListener') {
      return function addEventListener(...args) {
        signalHandled = true;
        return target[prop].apply(this, args);
      };
    } else {
      return Reflect.get(target, prop, receiver);
    }
  },
  set(target, prop, value) {
    if (prop === 'onabort') {
      signalHandled = true;
    }
    return Reflect.set(target, prop, value);
  }
});

// These 2 do not activate the `signalHandled`
signal.onabort;
signal.addEventListener;

signal.onabort = null;
signal.addEventListener('abort', () => {
});

console.log(signalHandled);

This ensures that we know if the signal has been handled. In this way it is possible to enforce that PromiseCancellable always has its signal handled at some point. And if users really want to prevent anything from happening, they just need to do signal.onabort = null.

[CMCDragonkai]

This is now done.

[CMCDragonkai]

Any further testing will need to occur in production now.

[CMCDragonkai]

Note that I switched to using constructors for centralising the default signal handler logic. Now promise cancellable will always have a default signal handler. The only way to prevent it is to supply your own custom handler, or within the executor, set the onabort to null.

new PromiseCancellable((resolve, reject, signal) => {
  signal.onabort = null;
});

Doing so will prevent the default signal handler.

I thought I could remove the symbol species and rely on the super.then using the PromiseCancellable constructor, however this broke the delayed cancellation tests. Couldn't figure out why, so I kept it as is, and all tests pass now.