Task.fromNodeBack is not handling cancellation

cdoublev commented 5 years ago

All of the Folktale task primitives should be dealing with cancellation.

Task.fromNodeBack currently doesn't prevent resolving|rejecting when the task is cancelled.

Steps to reproduce

Given:

const Task = require('folktale/concurrency/task')

const delay = Task.fromNodeback((result, error, cb) =>
    setTimeout(() => cb(error ? result : null, result)), 0)

This works:

Task.waitAll([delay(1, false), delay(2, true)])
   .orElse(e => Task.rejected(console.log('Error: ', e)))
   .run() // => Error 2

And this don't:

Task.waitAll([delay(1, false), delay(2, true), delay(3, true)])
   .orElse(e => Task.rejected(console.log('Error: ', e)))
   .run() // => Error 2
  // throw new Error('Only pending deferreds can be ' + description + ', this deferred is already ' + description + '.');

Expected behaviour

Last example shouldn't throw an error (Task.fromNodeback should check if task is already cancelled).

Observed behaviour

The error Only pending deferreds can be ' + description + ', this deferred is already ' + description + '.' is thrown.

Environment

OS: Vagrant Debian Box
JavaScript VM: node v11.6.0
Folktale version: 2.3.1

Additional information

robotlolita commented 5 years ago

@cdoublev can you install the beta release and check if it works for you?

$ npm install folktale@next

cdoublev commented 5 years ago

Yes, my own (rejection) tests stay green (without yelling about uncaught errors) after updating with @next and reverting all waitAll or and() usages from .apply(). Thank you for fixing/completing my tests. It perfectly answers my question asked in this comment. 👍

origamitower / folktale