Fixes #163; Created new windowMaid.js (#168)

* Fixes #163; Created new windowMaid.js and tested it to ensure more robust window destruction.  Destroyed windows can remain in memory, so that needs to be inspected.  Also allow clients to remove windows from the cache explicitly #163

* naming #163

* object literal instead of static functions #163

Author: codecounselor

lib/exportJob.js

@@ -16,15 +16,14 @@ const uuid = require('uuid')
 const debug = require('debug')
 const logger = debug('electronpdf:')
 
+const WindowMaid = require('./windowMaid')
 const wargs = require('./args')
 
 // CONSTANTS
 /** Used to calculate browser dimensions based on PDF size */
 const HTML_DPI = 96
 /** Interval for which to check for hung windows, in milliseconds */
 const HUNG_WINDOW_CLEANUP_INTERVAL = process.env.ELECTRONPDF_WINDOW_CLEANUP_INTERVAL || 1000 * 30 /* seconds */
-/** How long a window can remain open before it is terminated, in milliseconds */
-const HUNG_WINDOW_THRESHOLD = process.env.ELECTRONPDF_WINDOW_LIFE_THRESHOLD || 1000 * 60 * 5 /* minutes */
 /** Used to determine browser size using a Micron -> Inch -> Pixel conversion */
 const MICRONS_INCH_RATIO = 25400
 /** When a ready event option is set, this is the default timeout.  It is overridden by the wait option */
@@ -41,45 +40,8 @@ const DEFAULT_OPTIONS = {
   inMemory: false
 }
 
-// Window Cache - Keep track of all windows created, and if any get stuck close
-// them
-const windowCache = {}
-/**
- * When a job creates a window it invoks this method so any memory leaks
- * due to hung windows are prevented.  This can happen if an uncaught exception
- * occurs and job.destroy() is never invoked.
- * @param job
- */
-function registerOpenWindow (job) {
-  const w = job.window
-  windowCache[w.id] = {job: job, window: w, lastUsed: Date.now()}
-}
-/**
- * Anytime a window is used this function should be invoked to update
- * the lastUsed property in the window cache
- * @param id
- */
-function touchWindow (id) {
-  windowCache[id].lastUsed = Date.now()
-}
-
-function cleanupHungWindows () {
-  const now = Date.now()
-  const hungWindows = _.filter(windowCache,
-    e => now - e.lastUsed > HUNG_WINDOW_THRESHOLD)
-  logger(`checking hung windows.  total windows: ${_.size(windowCache)}`)
-  _.forEach(hungWindows, e => {
-    const windowContext = {
-      id: e.window.id,
-      lifespan: now - e.lastUsed
-    }
-    e.job.emit('window.termination', windowContext)
-    delete windowCache[e.window.id]
-    e.job.destroy()
-  })
-}
-
-setInterval(cleanupHungWindows, HUNG_WINDOW_CLEANUP_INTERVAL)
+// Use the maid to ensure we don't leak windows
+setInterval(WindowMaid.cleanupHungWindows, HUNG_WINDOW_CLEANUP_INTERVAL)
 
 /**
  * A job should be created to process a given export opreation for one or more
@@ -143,7 +105,7 @@ class ExportJob extends EventEmitter {
     this.emit(`${RENDER_EVENT_PREFIX}start`)
     this._launchBrowserWindow()
     const win = this.window
-    registerOpenWindow(this)
+    WindowMaid.registerOpenWindow(this)
 
     // TODO: Check for different domains, this is meant to support only a single origin
     const firstUrl = this.input[0]
@@ -227,6 +189,8 @@ class ExportJob extends EventEmitter {
   destroy () {
     if (this.window) {
       try {
+        logger(`destroying job with window: ${this.window.id}`)
+        WindowMaid.removeWindow(this.window.id)
         this.window.close()
       } finally {
         this.window = undefined
@@ -291,7 +255,7 @@ class ExportJob extends EventEmitter {
    * @private
    */
   _initializeWindowForResource () {
-    touchWindow(this.window.id)
+    WindowMaid.touchWindow(this.window.id)
     // Reset the generated flag for each input URL because this same job/window
     // can be reused in this scenario
     this.generated = false

lib/windowMaid.js

@@ -0,0 +1,86 @@
+// Third Party Modules
+const _ = require('lodash')
+
+// Logging
+const debug = require('debug')
+const logger = debug('electronpdf:')
+
+/** How long a window can remain open before it is terminated, in milliseconds */
+const HUNG_WINDOW_THRESHOLD = process.env.ELECTRONPDF_WINDOW_LIFE_THRESHOLD || 1000 * 60 * 5 /* minutes */
+
+// Window Cache - Keep track of all windows created, and if any get stuck close
+// them
+const windowCache = {}
+
+const windowMaid = {
+
+  /**
+   * When a job creates a window it invokes this method so any memory leaks
+   * due to hung windows are prevented.  This can happen if an uncaught
+   * exception occurs and job.destroy() is never invoked.
+   * @param exportJob the ExportJob instance, it must have a window reference set
+   */
+  registerOpenWindow (exportJob) {
+    const w = exportJob.window
+    windowCache[w.id] = {id: w.id, job: exportJob, window: w, lastUsed: Date.now()}
+  },
+
+  /**
+   * Anytime a window is used this function should be invoked to update
+   * the lastUsed property in the window cache
+   * @param id
+   */
+  touchWindow (id) {
+    windowCache[id].lastUsed = Date.now()
+  },
+
+  /**
+   * Allows a job to gracefully remove a window
+   * @param id the window id
+   */
+  removeWindow (id) {
+    delete windowCache[id]
+  },
+
+  /**
+   * Checks every window that was registered to make sure it has been used within
+   * the allowed threshold, and if not it is destroyed and removed from memory.
+   *
+   * @param {number} [threshold=process.env.ELECTRONPDF_WINDOW_LIFE_THRESHOLD]
+   * the number of millseconds that a window has to be open and untouched for it to be destroyed.
+   */
+  cleanupHungWindows (threshold) {
+    const now = Date.now()
+    const th = threshold || HUNG_WINDOW_THRESHOLD
+    const hungWindows = _.filter(windowCache, e => now - e.lastUsed >= th)
+
+    logger(`checking hung windows-> ` +
+      `total windows: ${_.size(windowCache)}, ` +
+      `hung windows: ${_.size(hungWindows)}, ` +
+      `threshold: ${th}`)
+
+    _.forEach(hungWindows, e => {
+      logger('destroying hung window: ', e.id)
+      const destroyable = e.job && e.window && !e.window.isDestroyed()
+      if (destroyable) {
+        const windowContext = {
+          id: e.window.id,
+          lifespan: now - e.lastUsed
+        }
+        e.job.emit('window.termination', windowContext)
+        delete windowCache[e.id]
+        e.job.destroy()
+      } else {
+        logger('Warning: a window was left in the cache that was already destroyed, do proper cleanup')
+        delete windowCache[e.id]
+      }
+    })
+  },
+
+  windowCount () {
+    return _.size(windowCache)
+  }
+
+}
+
+module.exports = windowMaid

test/windowMaid-test.js

@@ -0,0 +1,40 @@
+import { test } from 'ava'
+
+import WindowMaid from '../lib/windowMaid'
+
+test('_cleanupHungWindows cleans up non destroyed window', t => {
+  // Used to fail the test if emit() and destroy() are not invoked
+  t.plan(3)
+
+  let job = {
+    id: 'jobId',
+    window: {id: 1, isDestroyed: () => false},
+    emit: (event, context) => {
+      t.is(event, 'window.termination')
+    },
+    destroy: () => {
+      t.pass('we expected destruction and it happened!')
+    }
+  }
+
+  WindowMaid.registerOpenWindow(job)
+  WindowMaid.cleanupHungWindows(-1)
+  t.is(WindowMaid.windowCount(), 0)
+})
+
+test('_cleanupHungWindows handles destroyed window', t => {
+  let job = {
+    id: 'jobId',
+    window: {id: 1, isDestroyed: () => true},
+    emit: (event, context) => {
+      t.fail('no events were expected')
+    },
+    destroy: () => {
+      t.fail('Calling destroy on an already destroyed window is not allowed')
+    }
+  }
+
+  WindowMaid.registerOpenWindow(job)
+  WindowMaid.cleanupHungWindows(-1)
+  t.is(WindowMaid.windowCount(), 0)
+})