fix($q): call reject() even if $exceptionHandler rethrows

petebacondarwin · btford · commit d59027c40ed7 · 2013-08-12T16:23:37.000-07:00
Normally $exceptionHandler doesn't throw an exception.  It is normally
used just for logging and so on.  But if an application developer
implemented a version that did throw an exception then $q would never
have called reject() when converting an exception thrown inside a `then`
handler into a rejected promise.
diff --git a/src/ng/q.js b/src/ng/q.js
@@ -215,17 +215,17 @@ function qFactory(nextTick, exceptionHandler) {
             try {
               result.resolve((callback || defaultCallback)(value));
             } catch(e) {
-              exceptionHandler(e);
               result.reject(e);
+              exceptionHandler(e);
             }
           };
 
           var wrappedErrback = function(reason) {
             try {
               result.resolve((errback || defaultErrback)(reason));
             } catch(e) {
-              exceptionHandler(e);
               result.reject(e);
+              exceptionHandler(e);
             }
           };
 
diff --git a/test/ng/qSpec.js b/test/ng/qSpec.js
@@ -101,6 +101,7 @@ describe('q', function() {
       mockNextTick.queue.push(task);
     },
     queue: [],
+    logExceptions: true,
     flush: function() {
       if (!mockNextTick.queue.length) throw new Error('Nothing to be flushed!');
       while (mockNextTick.queue.length) {
@@ -110,7 +111,9 @@ describe('q', function() {
           try {
             task();
           } catch(e) {
-            dump('exception in mockNextTick:', e, e.name, e.message, task);
+            if ( mockNextTick.logExceptions ) {
+              dump('exception in mockNextTick:', e, e.name, e.message, task);
+            }
           }
         });
       }
@@ -836,4 +839,50 @@ describe('q', function() {
       });
     });
   });
+
+
+  describe('when exceptionHandler rethrows exceptions, ', function() {
+    var originalLogExceptions, deferred, errorSpy, exceptionExceptionSpy;
+
+    beforeEach(function() {
+      // Turn off exception logging for these particular tests
+      originalLogExceptions = mockNextTick.logExceptions;
+      mockNextTick.logExceptions = false;
+
+      // Set up spies
+      exceptionExceptionSpy = jasmine.createSpy('rethrowExceptionHandler')
+      .andCallFake(function rethrowExceptionHandler(e) {
+        throw e;
+      });
+      errorSpy = jasmine.createSpy('errorSpy');
+
+
+      q = qFactory(mockNextTick.nextTick, exceptionExceptionSpy);
+      deferred = q.defer();
+    });
+
+    
+    afterEach(function() {
+      // Restore the original exception logging mode
+      mockNextTick.logExceptions = originalLogExceptions;
+    });
+
+
+    it('should still reject the promise, when exception is thrown in success handler, even if exceptionHandler rethrows', function() {
+      deferred.promise.then(function() { throw 'reject'; }).then(null, errorSpy);
+      deferred.resolve('resolve');
+      mockNextTick.flush();
+      expect(exceptionExceptionSpy).toHaveBeenCalled();
+      expect(errorSpy).toHaveBeenCalled();
+    });
+    
+
+    it('should still reject the promise, when exception is thrown in success handler, even if exceptionHandler rethrows', function() {
+      deferred.promise.then(null, function() { throw 'reject again'; }).then(null, errorSpy);
+      deferred.reject('reject');
+      mockNextTick.flush();
+      expect(exceptionExceptionSpy).toHaveBeenCalled();
+      expect(errorSpy).toHaveBeenCalled();
+    });
+  });
 });
