Incorrect stack trace when coroutine throws sync and long stack traces enabled

overlookmotel commented 6 years ago

Please answer the questions the best you can:

1) What version of bluebird is the issue happening on?

v3.5.1

2) What platform and version? (For example Node.js 0.12 or Google Chrome 32)

Node 6.12.2, Mac OS 10.12.6

3) Did this issue happen with earlier version of bluebird?

Unknown

This is a very small nit.

When long stack traces are enabled, the stack trace for an error thrown synchronously in a coroutine includes "From previous event" when in fact everything happened synchronously.

const Promise = require('bluebird');
Promise.longStackTraces();

Promise.coroutine(function *() {
    throw new Error('foo');
})();

Produces error:

Error: foo
    at /test/test.js:5:8
    at next (native)
From previous event:
    at Object.<anonymous> (/test/test.js:6:3)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:383:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:496:3

Without long stack traces enabled, error trace is:

Error: foo
    at /test/test.js:5:8
    at next (native)
    at tryCatcher (/test/node_modules/bluebird/js/release/util.js:16:23)
    at PromiseSpawn._promiseFulfilled (/test/node_modules/bluebird/js/release/generators.js:97:49)
    at /test/node_modules/bluebird/js/release/generators.js:201:15
    at Object.<anonymous> (/test/test.js:6:3)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:383:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:496:3

The source of the problem appears to be this line where ._rejectCallback() is always called with synchronous = false regardless of whether error is thrown sync or not.

benjamingr commented 6 years ago

Honestly I'd expect .coroutine to always defer being called by a microtick. The surprising part to me here is that it doesn't do that.

overlookmotel commented 6 years ago

Do you mean that you'd expect the generator function not to be called immediately (synchronously) when the coroutine is called? i.e. you'd expect this:

const fn = Promise.coroutine( function*() {
   console.log('generator function called');
});

fn();
console.log('next line');

to output:

next line
generator function called

Personally, I feel the current behaviour is correct - that the generator function should be called synchronously. I believe this matches the behaviour of async/await.

overlookmotel commented 6 years ago

Thanks for swift reply, by the way.

benjamingr commented 6 years ago

It does match the behavior of async/await. I guess that it might as well be synchronous in the stack trace. @petkaantonov ?

petkaantonov / bluebird

Incorrect stack trace when coroutine throws sync and long stack traces enabled #1488