to throw or not to throw

[sonnyp]

I see 3 2 different approaches

callback

Pros

• Consistency between promise and callback styles
• Make it easier to handles all errors, no need for try catch, protect users/consumers from DOS accidents

Cons

• releases zalgo when the function is used with a callback but you can dezalgo that return cb

  function delay (time, cb) {
    return pg(function (done) {
      if (typeof time !== 'number') {
        return cb(new Error(time + ' is not a number'))
      }
      setTimeout(done, time)
    }, cb)
  }

  delay('123').then(fn) // does not throw, the promise will reject
  delay('123', fn) // does not throw, fn will be called with the error as first argument

Should polygoat dezalgo itself?

throw for callback, reject for promise

Pros

• This is how Node.js core behaves (doesn't mean it's the right way though see cons)

Cons

• Inconsistency (promise reject, callback throws)
• Doesn't protect from accidental DOS if you do not handles exceptions for the callback version

  function delay (time, cb) {
    return pg(function (done) {
      if (typeof time !== 'number') {
        throw new Error(time + ' is not a number')
      }
      setTimeout(done, time)
    }, cb)
  }

  delay('123').then(fn) // does not throw, the promise will reject
  delay('123', fn) // throws

throw

EDIT: discard, promise creation shouldn't throw

Pros

• works fine with with async/await

Cons

• the general consensus is that promise creation should not throw

  function delay (time, cb) {
    if (typeof time !== 'number') {
      throw new Error(time + ' is not a number')
    }
    return pg(function (done) {
      setTimeout(done, time)
    }, cb)
  }

  delay('123').then(fn) // throws
  delay('123', fn) // throws

[sonnyp]

cc @benjamingr any thought?

[benjamingr]

Some notes:

• Operational errors and programmer errors have nothing to do with async/sync (I can find a reference where I debate this with the person who wrote the Joyent error handling docs if you'd like). There was also discussion about this in nodejs/promises and in ESDiscuss.
• What is a good idea is to give errors wrapped through polygoat an indicator, bluebird adds a special property to these errors if it is able so they can be filtered and checked so programmer errors aren't caught by most handlers (again, this is not a sync/async thing - this is just to help consumers. Generally, the only one to specify what is a programmer and what is an operational error is the consumer.
• Your "callback" approach does not release Zalgo (how would it release Zalgo? then/catch handlers always execute when all other code has already finished.

The "throw for callback reject for promise" depends, if you are sure that it is a programmer error you can throw, otherwise it's better to use the err parameter to pass the error.

Since there is no unhandled error detection for callback err parameter it is important that you throw rather than pass the err parameter if there is reason to believe it is a programmer error.

[sonnyp]

Operational errors and programmer errors have nothing to do with async/sync (I can find a reference where I debate this with the person who wrote the Joyent error handling docs if you'd like). There was also discussion about this in nodejs/promises and in ESDiscuss.

If you have the references somewhere I'll be very interested.

Your "callback" approach does not release Zalgo (how would it release Zalgo? then/catch handlers always execute when all other code has already finished.

True for promise but it does release Zalgo when the "hybrid" function is used with a callback.

 'use strict'

 var pg = require('polygoat')

 function delay (time, cb) {
    return pg(function (done) {
      if (typeof time !== 'number') {
        return cb(new Error(time + ' is not a number'))
      }
      setTimeout(done, time)
    }, cb)
  }

  delay('123', function (err) {
    console.log(err)
  })

  console.log('w00t')

prints

[Error: 123 is not a number]
w00t

[sonnyp]

is there a consensus on whether promise instantiation may/should throw?

for example

delay('123') // throws

like in my "throw" approach example above

or maybe a interesting discussion on that topic somewhere? couldn't find anything

[benjamingr]

If you have the references somewhere I'll be very interested.

nodejs/docs#82
nodejs/promises#23
nodejs/promises#10

True for promise but it does release Zalgo when the "hybrid" function is used with a callback.

That's fair enough.

@benjamingr is there a consensus on whether promise creation can throw?

Promise creation shouldn't throw, it should reject, I can dig the discussion up if it's really important to you from the WHATWG mailing list when promises were introduced to the DOM. Note that in one very clear case - the promise constructor itself - passing invalid arguments does throw. I think this is because unhandled rejection detection didn't really exist back then.

[sonnyp]

I've updated the "throw for callback, reject for promise" approach with a much more convenient approach

[sonnyp]

thanks for the links, agree with you

Operational errors and programmer errors have nothing to do with async/sync

True, updated the "throw" approach

Promise creation shouldn't throw, it should reject

I'm all for choice but yeah I guess it would make everybody's life easier if all agreed on that.

Okay, discarded the approach "throw".

---

That leaves us with the "callback" and the "throw for callback, reject for promise" approaches.

I'd like to add a "error handling" section in the README.md of polygoat and I'm not sure which one (or both?) I should recommend. I guess you're favorite would be the "callback" approach.

Should polygoat dezelago itself for the "callback" approach? dezalgo module uses asap and code base is pretty big :/

I guess polygoat could simply use setImmediate || process.nextTick || postMessage || setTimeout OR use dezalgo on Node.js anything else available on browser (so that the browser file size doesn't increase with dezalgo source), the dezalgo function could also be provided as an optional parameter like the Promise impl

[sonnyp]

here is a proposal #10

[benjamingr]

Scheduling seems like a pretty nice task until you realize just how many schedulers you have to support to do a reasonable job.

If you want to "dezalgo" I'd do it by Promise.resolve.then(...) which already does dezalgo through a correct scheduler - but note that callback users wouldn't expect "dezalgo"ing to happen anyway.

[sonnyp]

If you want to "dezalgo" I'd do it by Promise.resolve.then(...) which already does dezalgo through a correct scheduler - but note that callback users wouldn't expect "dezalgo"ing to happen anyway.

the point of polygot is that it doesn't require Promise support/polyfill

[benjamingr]

@sonnyp right, but sometimes you don't have a choice and native promises are available and you have to use them to dezalgo. Ridiculous? Sure - just not as bad as users in iOS 8.3 not being able to use your code since timers are implicitly throttled in low battery mode and/or background. Fun.

[sonnyp]

setTimeout/postMessage is an other option

Yes, it's not as fast but in the browser who cares?

[benjamingr]

setTimeout sometimes doesn't get executed in some browsers (above I name iOS 8.3 in a background tab as an example).

It isn't slow (it is, but that's not the problem) - it breaks the functionality of the code.

[greim]

I think it's best when in doubt to not assume something is a programmer error. Polygoat could punt the question to library authors. If they do this:

function something(input, cb) {
  ...code that might throw...
  return pg(function (done) {
    eventuallyCall(done);
  }, cb)
}

...then they get the "throw for callback, reject for promise" behavior. Otherwise:

function something(input, cb) {
  return pg(function (done) {
    ...code that might throw...
    eventuallyCall(done);
  }, cb)
}

...then they get the "callback" behavior. In any case I agree if/whenever a callback is called, whether or not with an error, it should never happen before the current call stack bottoms out.

[sonnyp]

@greim

Your first example would throw for callback and for promise

If you want the callback version to throw, throw inside return pg(...) otherwise return cb(err)

Regarding dezalgo, not sure it should be in the scope of this module.