Pull requests

#9 Merged
Repository
samcday samcday
Branch
oauth-promise-wtf
Repository
atlassian atlassian
Branch
master

Fix for promise handler in oauth middleware.

Author
  1. Sam Day
Reviewers
Description

This fixes an issue in the oauth middleware that causes requests to hang indefinitely. I'll do my best to explain this one clearly ;)

Consider the following code snippet:

var RSVP = require('rsvp');

var promise = new RSVP.Promise(function(resolve,reject) {
    setTimeout(resolve, 1000);
});

promise.then(function() {
    throw new Error(":(");
}, function() {
    console.log("This won't be called.");
});

promise.then(function() {
    throw new Error(":(");
}).fail(function() {
    console.log("But this will.");
});

The problem here is that if a promise handler itself throws an error (NOT to be confused with the promise itself being rejected due to an error), the fail callback is not called if it's passed as the second argument to a single then(). Instead, you must chain a fail() after the original promise handler. If this doesn't make sense, let me know and I'll try explaining it a different way :)

Anyway, the issue I was having is that lib/internal/oauth.js itself is exploding (for reasons unknown, that may result in a separate PR once I nail that issue) in the exported verify() function. This causes the promise handler to throw an error, and the fail handler never gets called. Instead, the error is swallowed silently, the response isn't cleaned up nor is next() called. Yay async programming!

I didn't go hunting for this promise handler issue elsewhere in the code.

  • Learn about pull requests

Comments (3)

  1. Rich Manalang [Atlassian]

    Thanks for the PR @sday_atlassian. According to the RSVP docs, however, .then(success, failure) is equiv to .then(success).fail(failure). I haven't dug into their code to see why you're getting different results. Either way, this seems like a pretty safe PR.

  2. Sam Day author

    Rich Manalang [Atlassian] I think it is more to do with the fact that if you call .then(success, failure), that's the equivalent of saying this:

    var promiseA = somePromiseMethod();
    promiseA.then(/*...*/);
    promiseA.fail(/*...*/);
    

    Whereas if you do something like .then(successHandler).fail(failHandler) that looks more like this:

    var promiseA = somePromiseMethod();
    var promiseB = promiseA.then(/*...*/);
    promiseB.fail(/*...*/);
    

    In the latter case you can see that our fail handler is chained after the success handler, which means any errors in the success handler will be picked up by the fail handler. This is because .then() and .fail() themselves return new promises. Does that make sense?