Bluebird: Invalid `rejected with a non-error` warning

[vitaly-t]

When we set a message in out custom error object based on some non-string data, Bluebird displays Warning: a promise was rejected with a non-error: [object Object], which is clearly a mistake.

Here's complete example:

'use strict';

var util = require('util');
var promise = require('bluebird');

function CustomError(data) {
    this.message = data;
    Error.captureStackTrace(this, CustomError);
}

util.inherits(CustomError, Error);
CustomError.prototype.name = 'CustomError';

promise.reject(new CustomError([]))
    .catch(error=> {
        console.log(error);
    });

This should not produce that warning, but it does.

[benjamingr]

You haven't defined a .message string property on your error object. All error objects in ECMAScript have message string properties. Instead you passed it an array.

This breaks code that expects .message to be a string like it is on Error.prototype which is why bluebird rejects your error.

If you subclass Error with a message property which is a string then the error would not trigger.

Thanks for the report.

[vitaly-t]

Why wouldn't Bluebird just check if(error instanceof Error)?

[benjamingr]

Good question! Bluebird runs in multiple environments where that check does not make sense - here are some:

• Across iFrames.
• Across a page and a different page it opened.
• Across contexts in NodeJS (using vm).
• In electron between the node realm and browser realm.
• In extensions in Chrome and Firefox where the extension code and the browser code.

All those are examples where errors originate in different _realms_ which means instanceof Error wouldn't work. There is also a lot of difference between engines and in execution contexts like in Rhino.

There was an Error.isError proposal by @ljharb but I don't think that's being pursued.

[benjamingr]

I tried to talk @petkaantonov into doing || instanceof Error but he was against it by the way.

[ljharb]

The committee rejected Error.isError, but I'm working on a proposal for a method to get stack traces in a standard way that would allow for the same brand checking, and that should be polyfillable in most browsers - I'll let you know when I have one ready :-)

[vitaly-t]

I've killed a full day trying to adjust my custom error objects so that Bluebird wouldn't produce the warning, giving up now, it's not worth it.

I don't think it is a good idea to require that your custom error objects in Node.js can't be used for rejection, unless they do a few more tricks just to circumvent Bluebird. A bit of a hack this is.

I have a number of custom error types in my library, they are all properly derived from Error, they all produce error instanceof Error = true, and yet they all result in that warning from Bluebird.

[benjamingr]

It's not a hack, all error properties should behave like standard JavaScript errors which means they should include a .message property like every JavaScript error - I'm not sure how else to explain this.

[vitaly-t]

Well, I'm still trying to figure out what else is causing Bluebird to think that a custom error object isn't really an Error. That first example I provided was just one case, and apparently there is more, I just can't figure out yet what that is, something elusive...

[benjamingr]

function isError(obj) {
    return obj !== null &&
           typeof obj === "object" &&
           typeof obj.message === "string" &&
           typeof obj.name === "string";
}

[vitaly-t]

@benjamingr

Ok, more results, after a long digging into Bluebird.

Here's the reason all custom errors are failing:

    var hasStack = trace === reason;

It decides whether the object has stack based on this comparison, which from what I see is invalid. And then it produces a warning based on that.

I think it wasn't tested properly when a custom error is defined in one module, then used in another, and your library uses the last one, I am always getting the error object for which that condition trace === reason is false, hence the problem.

[spion]

No, ensureErrorObject returns the same argument that is passed when the object is successfully checked to be an error. That in turn does that if canAttachTrace returns true. And that is equivalent to:

function canAttachTrace(obj) {
    return isError(obj) && es5.propertyIsWritable(obj, "stack");
}

isError was pasted above by @benjamingr so the total conditions are:

• It has to be a non-null object
• It has to have name, message properties which are strings
• The stack property needs to be writable.

[vitaly-t]

No

LOL

Here I patched the Bluebird source code for some diagnostics:

Promise.prototype._rejectCallback =
function(reason, synchronous, ignoreNonErrorWarnings) {
    var trace = util.ensureErrorObject(reason);
    console.log("TRACE:", trace);
    console.log("REASON:", reason);
    console.log("EQUAL:", trace === reason);
    var hasStack = trace === reason;
    if (!hasStack && !ignoreNonErrorWarnings && debug.warnings()) {
        var message = "a promise was rejected with a non-error: " +
            util.classString(reason);
        this._warn(message, true);
    }
    this._attachExtraTrace(trace, synchronous ? hasStack : false);
    this._reject(reason);
};

It outputs:

TRACE: [Error: BatchError: ops]
REASON: { [BatchError: ops]
  data: [ { success: false, result: [Error: ops] } ],
  getErrors: [Function],
  first: [Error: ops],
  message: 'ops',
  stat: { total: 1, succeeded: 0, failed: 1, duration: 1 } }
EQUAL: false
Warning: a promise was rejected with a non-error: [object Object]
    at D:\NodeJS\tests\node_modules\pg-promise\lib\database.js:1195:31
    at processImmediate [as _immediateCallback] (timers.js:383:17)
From previous event:
    at Database.taskProcessor (D:\NodeJS\tests\node_modules\pg-promise\lib\database.js:1193:23)
    at Database.obj.task (D:\NodeJS\tests\node_modules\pg-promise\lib\database.js:1027:34)
    at test (D:\NodeJS\tests\test.js:62:8)
    at Object.<anonymous> (D:\NodeJS\tests\test.js:55:1)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:139:18)
    at node.js:968:3

And just to play on the safe side, inside the library all my unit tests now additionally check every error using this:

function isError(e, name) {
    var confirmed = e instanceof Error &&
        typeof e === 'object' &&
        typeof e.message === 'string' &&
        typeof e.stack === 'string';

    if (name === undefined) {
        confirmed = confirmed && typeof e.name === 'string';
    } else {
        confirmed = confirmed && e.name === name;
    }

    if (!confirmed) {
        if (!(e instanceof Error)) {
            console.error("ERROR: Not an Error instance.");
        }
        if (typeof e.message !== 'string') {
            console.error("ERROR: Message is a not a string:", e.message);
        }
        if (typeof e.stack !== 'string') {
            console.error("ERROR: Stack is a not a string:", e.stack);
        }
    }

    return confirmed;
}

So every error has 100% valid error signature:

• they are all instanceof Error
• stack, name and message are all set to correct strings everywhere

and yet Bluebird doesn't agree, based on that magic reason vs trace calculation.

[spion]

1. Bluebird calls var trace = util.ensureErrorObject(error). It works like this

   function ensureErrorObject(error) {
   if (isReallyError(error)) return error;
   else return wrapIntoActualError(error);
   }
   

2. Bluebird does a reference comparison to see if ensure returned the same object

   if (trace === error)
   

   This will only be true if the first branch was triggered i.e. if (isReallyError(error)) return error;

   The reason for this magic is that the cost of a reference comparison is much lower than checking the error object all over again.

My suggestion would be to make a minimal test case - that should make it easier to figure it out.

[vitaly-t]

As I explained earlier, when a custom error travels from one module dependency to another, your trace vs reason are no longer what you think they should be.

A "simple" test requires: declare module A with a custom error, then use it within module B, and have your client use that module B. I cannot replicate the problem within a single module.

I will try to make time later on, but it will take publishing two test modules into NPM.

[vitaly-t]

Ok, this is the last time I've spent so many hours trying to figure it out...

Now without the hypotheticals, only the real code...

I have module spex, that never throws any Bluebird warnings, works perfectly, has 100% test coverage.

I have module pg-promise, which is again, never throws any Bluebird warnings, works always perfectly, has 100% test coverage.

And yet, just as pg-promise attempts to use spex, any reject results in that Bluebird warning.

Now, considering that both modules separately never produce that warning on rejects, only when one is called from the other - that doesn't really make any sense to me.

A complete test application:

'use strict';

var promise = require('bluebird');
var pgp = require('pg-promise')({
    promiseLib: promise
});

var cn = "postgres://postgres@localhost:5432/pg_promise_test";
var db = pgp(cn);

function f() {
    throw new Error("ops");
}

db.task(function (t) {
    return t.batch([f]);
})
    .catch(function (error) {
        console.log(error);
    });

I've spent so much time, peeling through the layers of inter-dependence between the modules, but couldn't spot any kind of fault, because as I said, both modules work perfectly on their own.

---

However, what was interesting is that when I modify batch implementation to simply reject with the custom error, like this:

function batch(values, cb, config) {

    var $p = config.promise, $utils = config.utils;

    return $p.reject(new BatchError([], [], 123));

the warning is gone.

This tells me, there is something within the method implementation that even though works perfectly, and never produces that Bluebird warning, starts producing the warning when used through another module.

Considering that the method employs promise pseudo-recursion, I am inclined to think something there changes the stack tracing that Bluebird fails to recognize.

[spion]

Your example doesn't currently generate warnings (spex 1.0.10, pg-promise 5.0.3). Which versions of spex and pg-promise does it use?

[vitaly-t]

I am using those very versions, under Node.js 4.4.5 and 6.2.2, under Windows 10, 64-bit.

I am getting those warnings in that example 100% of a time.

The exact output from that example:

(node:6664) Warning: a promise was rejected with a non-error: [object Object]
(node:6664) Warning: a promise was rejected with a non-error: [object Object]
BatchError {
    message: "ops"
    stat: { total: 1, succeeded: 0, failed: 1, duration: 2 }
    data: [
        0: {
            success: false
            result: [Error: ops]
                at Task.f (D:\NodeJS\tests\test3.js:12:11)
                at loop (D:\NodeJS\tests\node_modules\spex\lib\utils\index.js:47:72)
                at Task.resolve (D:\NodeJS\tests\node_modules\spex\lib\utils\index.js:69:9)
                at Promise._execute (D:\NodeJS\tests\node_modules\bluebird\js\release\debuggability.js:272:9)
                at Promise._resolveFromExecutor (D:\NodeJS\tests\node_modules\bluebird\js\release\promise.js:475:18)
                at new Promise (D:\NodeJS\tests\node_modules\bluebird\js\release\promise.js:77:14)
                at promise (D:\NodeJS\tests\node_modules\spex\lib\index.js:93:24)
                at Task.batch (D:\NodeJS\tests\node_modules\spex\lib\ext\batch.js:83:12)
                at Task.<anonymous> (D:\NodeJS\tests\node_modules\spex\lib\ext\batch.js:162:26)
                at Task.batch (D:\NodeJS\tests\node_modules\pg-promise\lib\task.js:138:39)
                at Task.<anonymous> (D:\NodeJS\tests\test3.js:16:14)
                at callback (D:\NodeJS\tests\node_modules\pg-promise\lib\task.js:191:25)
                at Function.Task.exec (D:\NodeJS\tests\node_modules\pg-promise\lib\task.js:291:12)
        }
    ]
}

[spion]

Remove Object.freeze(e) from lib/ext/batch.js - bluebird expects that the stack property is writable when longStackTraces are on.

(You'll probably need to do it for lib/ext/sequence.js too, where its repeated as Object.freeze(error))

To make it easier to debug this, the freezing concern that modifies the error object should have been a part of the error unit/submodule. That way unit tests will be able to catch this.

Not sure whats going on other than that, when you run this in normal code that doesn't attempt to do so much wrapping of bluebird behaviour

Promise.resolve()
.then(function() {
  var e = new Error();
  Object.freeze(e);
  throw e;
}).catch(function(error) {
  console.log("Caught: ", error.stack);
});

you get a clearer error:

TypeError: Cannot redefine property: stack
    at Object.defineProperty (native)
    at Object.notEnumerableProp (/node_modules/bluebird/js/release/util.js:97:9)
    at CapturedTrace.attachExtraTrace (/node_modules/bluebird/js/release/debuggability.js:725:10)
    at Promise.longStackTracesAttachExtraTrace [as _attachExtraTrace] (/node_modules/bluebird/js/release/debuggability.js:379:19)
    at Promise._rejectCallback (/node_modules/bluebird/js/release/promise.js:466:10)
    at Promise._settlePromiseFromHandler (/node_modules/bluebird/js/release/promise.js:513:17)
    at Promise._settlePromise (/node_modules/bluebird/js/release/promise.js:561:18)
    at Promise._settlePromiseCtx (/node_modules/bluebird/js/release/promise.js:598:10)
    at Async._drainQueue (/node_modules/bluebird/js/release/async.js:143:12)
    at Async._drainQueues (/node_modules/bluebird/js/release/async.js:148:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/node_modules/bluebird/js/release/async.js:17:14)
    at tryOnImmediate (timers.js:543:15)
    at processImmediate [as _immediateCallback] (timers.js:523:5)

[vitaly-t]

@spion wow! great job, man, right to the point!

So Bluebird fails to do detection for read-only Error objects! That's quite unexpected.

Thank you so much for your help!

Should it be made part of Bluebird documentation that it cannot deal with read-only errors?

---

Something strange through - why doesn't it complain when I use spex module directly?

[spion]

Not exactly unexpected. This only happens when longStackTraces is on, in which case bluebird must modify the stack property to add to the stack trace. In normal situations you'll get a clearer error, however when the above type error occurs, spex converts it again to a BatchError which fails to pass the isError test. I don't know where that happens in spex.

[vitaly-t]

If I change the test to use spex directly:

'use strict';

var promise = require('bluebird');
var pgp = require('pg-promise')({
    promiseLib: promise
});

var cn = "postgres://postgres@localhost:5432/pg_promise_test";
var db = pgp(cn);

function f() {
    throw new Error("ops");
}

pgp.spex.batch([f])
    .catch(function (error) {
        console.log(error);
    });

then it doesn't care whether Error is read-only or not.

[spion]

What you are doing is reject(e) then Object.freeze(e). Essentially this mutates the error object after passing it to bluebird. Is there any reason for this reversed behaviour?

[spion]

The above code you pasted produces

TypeError: Cannot redefine property: stack

when longStackTraces is on.

[vitaly-t]

The above code you pasted produces

Do you mean when the Object.freeze is used?

But it's all good without freezing the error?

[vitaly-t]

I think I will add some checks for Bluebird specifically. That's why I've just asked this question: Bluebird detection.

[spion]

@vitaly-t Just don't freeze the error. At the very least, leave the stack property writable, or don't turn on longStackTraces.

By the way, I already wrote about this restriction yesterday, when you only quoted "No" from that text and LOLed. Last restriction was "the stack property needs to be writable"

https://github.com/petkaantonov/bluebird/issues/1146#issuecomment-228826885

This should definitely be in the docs, in some form.

[vitaly-t]

or don't turn on longStackTraces.

That's out of my hands, as the library simply consumes the promise library, which is configurable by the client. I will likely just remove the object freezing.

Thank you so much for your help in this!