Prevent onCancel race conditions

[atticoos]

The goal of this PR is to avoid onCancel become called when a Promise does not actually become cancelled.

In Speculation's current form, onCancel will become invoked regardless of the outcome of the observed promise. This callback is useful to to perform cleanup tasks, but it might be used to perform other duties that should only be performed when a promise is indeed cancelled early.

This solution will avoid triggering onCancel unless the cancellation is actually performed before the pending promise becomes completed.

The solution works by creating a higher order function of resolve that lets speculation know if the Promise becomes resolved or rejected downstream in where the promise is managed by the dependent's business logic. The higher order function will maintain a boolean state of completed. Unfortunately this is a bit of an impure approach, as we manage state within speculation on the "progress" of the promise -- but I wasn't quite sure off the top of my head of a better way. Happy to revise if requested.

Example:

const speculation = require('./src/index');

const wait = (time, cancel) => {
  return speculation((resolve, reject, onCancel) => {
    var timer = setTimeout(resolve, time);

    onCancel(() => {
      console.log('onCancel invoked');
      clearTimeout(timer);
      reject(new Error('Cancelled'));

      // Perhaps report to something upstream in our application that
      // the pending promise has failed to complete properly.
    });
  }, cancel);
}

wait(100, wait(200))
  .then(() => console.log('Our promise has completed'));

Example output:

With this new patch:

[ericelliott]

Hi @ajwhite -- thanks for the PR.

Did you try running the unit test before adding the changes?

It passes with no code changes. Are you using TDD? (Write the test first).

[atticoos]

I had hand written some examples when experimenting with this library, which had lead to the reproducible behavior. However, I see where things went wrong in my test, and I had not written it first (good call). cancel doesn't fire until 100ms, yet the test completes at 50ms, so onCancel would not have been called by the time of assertion. Just need to block the final then where the assertion takes place for another 51ms.

Change incoming.

[atticoos]

The new test can now be seen failing against master in https://github.com/ajwhite/speculation/tree/unit-test-without-cancellation-patch

[ericelliott]

This test gets the job done:

test('speculation promise resolved before cancel', assert => {
  const msg = 'onCancel should not run';

  const testWait = (
    time,
    cancel = Promise.resolve()
  ) => speculation((resolve, reject, onCancel) => {
    onCancel(() => {
      assert.fail(msg);
    })
    .catch(() => wait(40))
    .then(() => {
      assert.end();
    });
  }, cancel);

  testWait(20, wait(50)).then(() => {
    assert.pass(msg);
  }).then(wait(40)).then(() => {
    assert.end();
  });
});

[atticoos]

Failing right inside the cancellation callback -- fantastic. No need for an additional dep 👍

[atticoos]

Okay, this looks good to me. Only issue in your feedback testWait was missing a resolve call

test('speculation promise resolved before cancel', assert => {
  const msg = 'onCancel should not run';

  const testWait = (
    time,
    cancel = Promise.resolve()
  ) => speculation((resolve, reject, onCancel) => {
    onCancel(() => {
      assert.fail(msg);
    })
    .catch(() => wait(40))
    .then(() => {
      assert.end();
    });
+  setTimeout(resolve, time);
  }, cancel);

  testWait(20, wait(50)).then(() => {
    assert.pass(msg);
  }).then(wait(40)).then(() => {
    assert.end();
  });
});

With the resolve in place, both end() will become called, and tape will yell at you

not ok 8 .end() called twice

I've moved this to only end in the completion block to the end of testWait -- I think that should be enough since that will be the last execution in the test.

However, I wonder if we should add a .catch before the end of testWait to ensure assert.end() is always called

 testWait(20, wait(50)).then(() => {
   assert.pass(msg);
 })
+.catch(() => {})
 .then(wait(40))
 .then(() => {
   assert.end();
 });

[ericelliott]

Should this work?

test('speculation promise resolved before cancel', assert => {
  const msg = 'onCancel should not run';

  const testWait = (
    time,
    cancel = Promise.resolve()
  ) => speculation((resolve, reject, onCancel) => {
    onCancel(() => {
      assert.fail(msg);
    });
  }, cancel);

  testWait(20, wait(50)).then(() => {
    assert.pass('success callback should be called');
  });

  wait(60).then(() => {
    assert.end();
  });
});

[atticoos]

Breaking it out like that greatly simplifies this and is a bit more coherent 👍

[ericelliott]

Cool, but it fails. 👎

[atticoos]

You're missing a setTimeout(resolve, time) in the speculation block -- works fine once added

[ericelliott]

I added some style tweaks to your PR. Thoughts?

[atticoos]

Looks good, I agree with these on a number of levels:

• handleCancelIfCompleted was a leaky abstraction in the naming
• Underscoring the lower order methods makes things clearer

Although isFulfilled -- "fulfilled" is the terminology as described in the spec for resolved promises. This represents both rejections or resolved. A "pending promise" becomes a "completed promise" in either fulfilled or rejected states.

But overall great improvements 👍

[ericelliott]

Oops, you're right. That should be isSettled.

[ericelliott]

Published in 1.0.5