Bluebird: There should be a way to generate warnings when reject is called after resolve

[aalexgabi]

I have lost many hours investigating a problem that arrived due to a promise being resolved and rejected afterward. Here is an example to illustrate the problem I had:

var Promise = require('bluebird')

new Promise(function (resolve, reject) {
    console.log('resolving')
    resolve()

    setTimeout(function () {
        console.log('rejecting')
        reject(new Error())
    }, 0)
}).then(function () {
    console.log('resolved')
}).catch(function (err) {
    console.log('rejected', err)
})

I tried to activate warnings but I din't get any:

root@874aba35616a:/tmp# BLUEBIRD_WARNINGS=1 node example.js 
resolving
rejecting
resolved
root@874aba35616a:/tmp# 

When I don't supply an error to reject, I get a warning so I'm confident that the warning mechanism gets activated using the environment variable.

Is there a way to have these warnings? If not can you add this feature? I bet it would save a lot of debugging time.

[spion]

Can you post some realistic examples where this happens? new Promise should be used very sparingly. Especially when dealing with existing promises, promise transformation functions should always be preferred, and when dealing with callback-style functions, promisify / promisifyAll should be used.

[benjamingr]

I actually use this fact as a feature (resolving after rejecting)

[aalexgabi]

Here is the code I was debugging:

return new Promise(function (resolve, reject) {
        var csvStream = csv.parse({ delimiter: self.request.separator || ',', columns: true, relax_column_count: true });
        csvStream.on('pipe', function (src) {
            src.on('data', function (data) { self.progressSize += data.length; });
        });
        self.sftpConnection.readAndPipeFile(file.filename, csvStream).catch(function(err){
            err.context = err.context || {};
            err.context.filename = file.filename
            reject(err);
        });
       // BulkPromiseStream is a transformer that waits for promise returned by the mapper to fulfill before acknowledging the fake "write" and emits error on rejection
        csvStream.pipe(new streamTools.BulkPromiseStream(function(slice) {
            return self.processSomeRows(slice).then(function (sliceReport) {
                addReports(report, sliceReport);
                lines += slice.length;
            }).then(function () {
                return self.updateProgress({
                    totalFiles: totalFiles,
                    processedFiles: fileNumber,
                    totalSize: self.totalSize,
                    processedSize: self.progressSize,
                    currentProcessedLines: lines,
                    currentReport: JSON.stringify(report),
                });
            });
        }))
            // we need to add a data consumer in order to finish the stream
            // if we don't, the end event is never called :(
            // it's because BulkPromiseStream is based on Transform
            .on('data', function() {})
            .on('end', resolve)
            .on('error', reject);
        csvStream.on('error', reject);
    })

The bug was in self.processSomeRows that returned too quickly for the last chunk. That resolved the promise but the previous chunk was generating an error later on. This error was totally invisible.

[benjamingr]

@aalexgabi have you tried using the monitoring API to debug it?

[aalexgabi]

@benjamingr No. I didn't know where the problem came from. A warning on stderr would have been very useful.

[benjamingr]

I tend to agree - PR welcome.

[sorccu]

Not a huge fan of this suggestion, though I understand the sentiment. Current warnings cover things that are almost certainly bugs and should be rewritten. This one is working as intended, and commonly relied upon (though usually in different forms). I personally feel that in a large application the proposed warning may in fact hinder debuggability, as it may bring out too many false positives. To avoid the warning, one would have to rewrite code that is already working as intended specifically just to get rid of that warning, which isn't productive. Additionally, it is not even clear how one might rewrite the original example to avoid the warning, and having a proposed solution should in my opinion be a basic requirement of adding one.

Finally, looking at the "real world" case, streams are notoriously difficult to get right, and I don't think it's bluebird's responsibility to help one avoid race conditions there :)

Either way maybe this issue should be closed.

[petkaantonov]

Agree with @sorccu