Merge pull request #545 from apifytech/fix/puppeteer-active-pages-cou…

…nting PuppeteerPool - Fixed the activePages counting.
apify · Jan 8, 2020 · db460f5 · db460f5
2 parents 35d769a + 9e627b7
commit db460f5
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 20 deletions.
diff --git a/src/puppeteer_pool.js b/src/puppeteer_pool.js
@@ -337,24 +337,6 @@ class PuppeteerPool {
             if (!instance.killed) log.error('PuppeteerPool: Puppeteer sent "disconnect" event. Maybe it crashed???', { id });
             this._retireInstance(instance);
         });
-        // This one is done manually in Puppeteerpool.newPage() so that it happens immediately.
-        // browser.on('targetcreated', () => instance.activePages++);
-        browser.on('targetdestroyed', (target) => {
-            // The event is also called for service workers and Chromium extensions, which must be ignored!
-            const type = target.type();
-            if (type !== 'page' && type !== 'other') return;
-
-            instance.activePages--;
-
-            if (instance.activePages === 0 && this.retiredInstances[id]) {
-                // Run this with a delay, otherwise page.close() that initiated this 'targetdestroyed' event
-                // might fail with "Protocol error (Target.closeTarget): Target closed."
-                setTimeout(() => {
-                    log.debug('PuppeteerPool: Killing retired browser because it has no active pages', { id });
-                    this._killInstance(instance);
-                }, PAGE_CLOSE_KILL_TIMEOUT_MILLIS);
-            }
-        });
     }
 
     /**
@@ -554,18 +536,18 @@ class PuppeteerPool {
      * @ignore
      */
     _decoratePage(page) {
+        const instance = this.pagesToInstancesMap.get(page);
+
         const originalPageClose = page.close;
         page.close = async (...args) => {
             this.closedPages.add(page);
             await originalPageClose.apply(page, args)
                 .catch((err) => {
-                    const instance = this.pagesToInstancesMap.get(page);
                     log.debug('PuppeteerPool: Page.close() failed', { errorMessage: err.message, id: instance.id });
                 });
             const context = page.browserContext();
             if (context.isIncognito()) {
                 await context.close().catch((err) => {
-                    const instance = this.pagesToInstancesMap.get(page);
                     log.debug('PuppeteerPool: Context.close() failed', { errorMessage: err.message, id: instance.id });
                 });
             }
@@ -575,6 +557,12 @@ class PuppeteerPool {
             log.exception(error, 'PuppeteerPool: Page crashed.');
             page.close();
         });
+
+        page.once('close', () => {
+            instance.activePages--;
+            this._killInstanceWithNoPages(instance);
+        });
+
         return page;
     }
 
@@ -750,6 +738,18 @@ class PuppeteerPool {
             }
         }
     }
+
+    _killInstanceWithNoPages(instance) {
+        const { id } = instance;
+        if (instance.activePages === 0 && this.retiredInstances[id]) {
+            // Run this with a delay, otherwise page.close()
+            // might fail with "Protocol error (Target.closeTarget): Target closed."
+            setTimeout(() => {
+                log.debug('PuppeteerPool: Killing retired browser because it has no active pages', { id });
+                this._killInstance(instance);
+            }, PAGE_CLOSE_KILL_TIMEOUT_MILLIS);
+        }
+    }
 }
 
 export default PuppeteerPool;
diff --git a/test/puppeteer_pool.test.js b/test/puppeteer_pool.test.js
@@ -230,6 +230,30 @@ describe('PuppeteerPool', () => {
         await pool.destroy();
     });
 
+    test('does not create more than maxOpenPagesPerInstance', async () => {
+        const pool = new Apify.PuppeteerPool({
+            maxOpenPagesPerInstance: 2,
+            retireInstanceAfterRequestCount: 100,
+        });
+        const opennedPages = [];
+
+        for (let i = 0; i < 12; i++) {
+            opennedPages.push(pool.newPage());
+        }
+        await Promise.all(opennedPages);
+
+        const instances = Object.values(pool.activeInstances);
+
+        expect(instances.length).toEqual(6);
+
+        instances.forEach((instance) => {
+            expect(instance.activePages).toEqual(2);
+        });
+
+        // Cleanup everything.
+        await pool.destroy();
+    });
+
     test('supports recycleDiskCache option', async () => {
         const pool = new Apify.PuppeteerPool({
             maxOpenPagesPerInstance: 1,