Merge pull request #699 from hapijs/domain-leak-and-cleanup-crash

Fix a domain leak and onCleanup failures
hapijs · Apr 29, 2017 · fcf8397 · fcf8397
2 parents bcecf84 + 1968a7c
commit fcf8397
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 7 deletions.
diff --git a/lib/runner.js b/lib/runner.js
@@ -452,7 +452,7 @@ internals.executeTests = function (experiment, state, skip, callback) {
                     state.report.tests.push(test);
                     state.reporter.test(test);
 
-                    return next();
+                    setImmediate(next);
                 });
             },
             function (next) {
@@ -610,6 +610,7 @@ internals.createDomainErrorHandler = (item, state, domain, domains, finish) => {
 
 internals.protect = function (item, state, callback) {
 
+    const finished = Hoek.once(callback);
     let isFirst = true;
     let timeoutId;
     let countBefore = -1;
@@ -625,6 +626,8 @@ internals.protect = function (item, state, callback) {
     // experiments.
     const domains = state.domains;
 
+    let cleaningUp = false;
+
     const finish = function (err, cause) {
 
         clearTimeout(timeoutId);
@@ -659,7 +662,8 @@ internals.protect = function (item, state, callback) {
         }
 
         if (!isFirst) {
-            const message = `Multiple callbacks or thrown errors received in test "${item.title}" (${cause})`;
+            const prefix = cleaningUp ? 'An error probably occured while cleaning up' : 'Multiple callbacks or thrown errors received in';
+            const message = `${prefix} test "${item.title}" (${cause})`;
 
             if (err && !/^Multiple callbacks/.test(err.message)) {
                 err.message = message + ': ' + err.message;
@@ -669,10 +673,20 @@ internals.protect = function (item, state, callback) {
             }
 
             state.report.errors.push(err);
+
+            if (cleaningUp) {
+                cleaningUp = false;
+                finished(err);
+            }
+
             return;
         }
         isFirst = false;
-        internals.cleanupItem(err, item, activeDomain, callback);
+        internals.cleanupItem(err, item, activeDomain, (err) => {
+
+            cleaningUp = false;
+            finished(err);
+        });
     };
 
     const ms = item.options.timeout !== undefined ? item.options.timeout : state.options.timeout;
@@ -699,17 +713,23 @@ internals.protect = function (item, state, callback) {
 
     setImmediate(() => {
 
+        const exitDomain = Hoek.once(() => domain.exit());
         domain.enter();
 
         item.onCleanup = null;
         const onCleanup = (func) => {
 
-            item.onCleanup = func;
+            item.onCleanup = (cb) => {
+
+                cleaningUp = true;
+                domain.run(func, cb);
+            };
         };
 
         const done = (err) => {
 
-            finish(err, 'done');
+            exitDomain();
+            setImmediate(finish, err, 'done');
         };
 
         item.notes = [];
@@ -723,13 +743,13 @@ internals.protect = function (item, state, callback) {
         if (itemResult &&
             itemResult.then instanceof Function) {
 
-            itemResult.then(() => finish(null), (err) => finish(err, 'done'));
+            itemResult.then(done, done);
         }
         else if (!item.fn.length) {
             finish(new Error(`Function for "${item.title}" should either take a callback argument or return a promise`), 'function signature');
         }
 
-        domain.exit();
+        exitDomain();
     });
 };
 

diff --git a/test/runner.js b/test/runner.js
@@ -126,6 +126,35 @@ describe('Runner', () => {
         });
     });
 
+    it('calls failing cleanup function', (done) => {
+
+        const script = Lab.script();
+
+        script.test('a', (done, onCleanup) => {
+
+            setImmediate(() => {
+
+                onCleanup((next) => {
+
+                    setImmediate(() => {
+
+                        throw new Error('oops');
+                    });
+                });
+
+                setImmediate(done);
+            });
+        });
+
+        Lab.execute(script, {}, null, (err, notebook) => {
+
+            expect(err).not.to.exist();
+            expect(notebook.tests).to.have.length(1);
+            expect(notebook.failures).to.equal(1);
+            done();
+        });
+    });
+
     it('should fail test that neither takes a callback nor returns anything', (done) => {
 
         const script = Lab.script({ schedule: false });