Merge pull request #679 from mrfelton/fix/grpc-disconnect

fix(grpc): ensure full disconnect on window close

app/lib/lnd/config.js

@@ -141,7 +141,9 @@ class LndConfig {
       },
       binaryPath: {
         enumerable: true,
-        value: binaryPath
+        get() {
+          return binaryPath()
+        }
       },
       dataDir: {
         enumerable: true,
@@ -152,13 +154,13 @@ class LndConfig {
       configPath: {
         enumerable: true,
         get() {
-          return join(appRootPath, 'resources', 'lnd.conf')
+          return join(appRootPath(), 'resources', 'lnd.conf')
         }
       },
       rpcProtoPath: {
         enumerable: true,
         get() {
-          return join(appRootPath, 'resources', 'rpc.proto')
+          return join(appRootPath(), 'resources', 'rpc.proto')
         }
       },
 

app/lib/lnd/lightning.js

@@ -1,5 +1,9 @@
+// @flow
+
 import grpc from 'grpc'
 import { loadSync } from '@grpc/proto-loader'
+import { BrowserWindow } from 'electron'
+import LndConfig from './config'
 import { getDeadline, validateHost, createSslCreds, createMacaroonCreds } from './util'
 import methods from './methods'
 import { mainLog } from '../utils/log'
@@ -7,11 +11,22 @@ import subscribeToTransactions from './subscribe/transactions'
 import subscribeToInvoices from './subscribe/invoices'
 import subscribeToChannelGraph from './subscribe/channelgraph'
 
+// Type definition for subscriptions property.
+type LightningSubscriptionsType = {
+  channelGraph: any,
+  invoices: any,
+  transactions: any
+}
+
 /**
  * Creates an LND grpc client lightning service.
  * @returns {Lightning}
  */
 class Lightning {
+  mainWindow: BrowserWindow
+  lnd: any
+  subscriptions: LightningSubscriptionsType
+
   constructor() {
     this.mainWindow = null
     this.lnd = null
@@ -26,7 +41,7 @@ class Lightning {
    * Connect to the gRPC interface and verify it is functional.
    * @return {Promise<rpc.lnrpc.Lightning>}
    */
-  async connect(lndConfig) {
+  async connect(lndConfig: LndConfig) {
     const { rpcProtoPath, host, cert, macaroon } = lndConfig
 
     // Verify that the host is valid before creating a gRPC client that is connected to it.
@@ -74,37 +89,40 @@ class Lightning {
    */
   disconnect() {
     this.unsubscribe()
-    this.lnd.close()
+    if (this.lnd) {
+      this.lnd.close()
+    }
   }
 
   /**
    * Hook up lnd restful methods.
    */
-  lndMethods(event, msg, data) {
+  lndMethods(event: Event, msg: string, data: any) {
     return methods(this.lnd, mainLog, event, msg, data)
   }
 
   /**
    * Subscribe to all bi-directional streams.
    */
-  subscribe(mainWindow) {
+  subscribe(mainWindow: BrowserWindow) {
     this.mainWindow = mainWindow
-    this.subscriptions.channelGraph = subscribeToChannelGraph(this.mainWindow, this.lnd, mainLog)
+
-    this.subscriptions.invoices = subscribeToInvoices(this.mainWindow, this.lnd, mainLog)
+    this.subscriptions.channelGraph = subscribeToChannelGraph.call(this)
-    this.subscriptions.transactions = subscribeToTransactions(this.mainWindow, this.lnd, mainLog)
+    this.subscriptions.invoices = subscribeToInvoices.call(this)
+    this.subscriptions.transactions = subscribeToTransactions.call(this)
   }
 
   /**
    * Unsubscribe from all bi-directional streams.
    */
   unsubscribe() {
+    this.mainWindow = null
     Object.keys(this.subscriptions).forEach(subscription => {
       if (this.subscriptions[subscription]) {
         this.subscriptions[subscription].cancel()
         this.subscriptions[subscription] = null
       }
     })
-    this.mainWindow = null
   }
 }
 

app/lib/lnd/subscribe/channelgraph.js

@@ -1,14 +1,21 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToChannelGraph(mainWindow, lnd, log) {
+export default function subscribeToChannelGraph() {
-  const call = lnd.subscribeChannelGraph({})
+  const call = this.lnd.subscribeChannelGraph({})
 
-  call.on('data', channelGraphData => mainWindow.send('channelGraphData', { channelGraphData }))
+  call.on('data', channelGraphData => {
-  call.on('end', () => log.info('end'))
+    if (this.mainWindow) {
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
+      this.mainWindow.send('channelGraphData', { channelGraphData })
-  call.on('status', channelGraphStatus =>
+    }
-    mainWindow.send('channelGraphStatus', { channelGraphStatus })
+  })
-  )
+  call.on('end', () => mainLog.info('end'))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
+  call.on('status', channelGraphStatus => {
+    if (this.mainWindow) {
+      this.mainWindow.send('channelGraphStatus', { channelGraphStatus })
+    }
+  })
 
   return call
 }

app/lib/lnd/subscribe/invoices.js

@@ -1,15 +1,18 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToInvoices(mainWindow, lnd, log) {
+export default function subscribeToInvoices() {
-  const call = lnd.subscribeInvoices({})
+  const call = this.lnd.subscribeInvoices({})
 
   call.on('data', invoice => {
-    log.info('INVOICE:', invoice)
+    mainLog.info('INVOICE:', invoice)
-    mainWindow.send('invoiceUpdate', { invoice })
+    if (this.mainWindow) {
+      this.mainWindow.send('invoiceUpdate', { invoice })
+    }
   })
-  call.on('end', () => log.info('end'))
+  call.on('end', () => mainLog.info('end'))
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
-  call.on('status', status => log.info('INVOICE STATUS:', status))
+  call.on('status', status => mainLog.info('INVOICE STATUS:', status))
 
   return call
 }

app/lib/lnd/subscribe/transactions.js

@@ -1,15 +1,18 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToTransactions(mainWindow, lnd, log) {
+export default function subscribeToTransactions() {
-  const call = lnd.subscribeTransactions({})
+  const call = this.lnd.subscribeTransactions({})
 
   call.on('data', transaction => {
-    log.info('TRANSACTION:', transaction)
+    mainLog.info('TRANSACTION:', transaction)
-    mainWindow.send('newTransaction', { transaction })
+    if (this.mainWindow) {
+      this.mainWindow.send('newTransaction', { transaction })
+    }
   })
-  call.on('end', () => log.info('end'))
+  call.on('end', () => mainLog.info('end'))
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
-  call.on('status', status => log.info('TRANSACTION STATUS: ', status))
+  call.on('status', status => mainLog.info('TRANSACTION STATUS: ', status))
 
   return call
 }

app/lib/lnd/util.js

@@ -33,8 +33,9 @@ const dnsLookup = promisify(dns.lookup)
  * And no, we can't just set our working directory to somewhere inside the asar. The OS can't handle that.
  * @return {String} Path to the lnd binary.
  */
-export const appRootPath =
+export const appRootPath = () => {
-  app.getAppPath().indexOf('default_app.asar') < 0 ? normalize(`${app.getAppPath()}/..`) : ''
+  return app.getAppPath().indexOf('default_app.asar') < 0 ? normalize(`${app.getAppPath()}/..`) : ''
+}
 
 /**
  * Get the OS specific lnd binary name.
@@ -46,9 +47,11 @@ export const binaryName = platform() === 'win32' ? 'lnd.exe' : 'lnd'
  * Get the OS specific path to the lnd binary.
  * @return {String} Path to the lnd binary.
  */
-export const binaryPath = isDev
+export const binaryPath = () => {
-  ? join(dirname(require.resolve('lnd-binary/package.json')), 'vendor', binaryName)
+  return isDev
-  : join(appRootPath, 'bin', binaryName)
+    ? join(dirname(require.resolve('lnd-binary/package.json')), 'vendor', binaryName)
+    : join(appRootPath(), 'bin', binaryName)
+}
 
 // ------------------------------------
 // Helpers

app/lib/zap/controller.js

@@ -199,9 +199,6 @@ class ZapController {
     if (this.neutrino) {
       this.neutrino.stop()
     }
-
-    // Give the grpc connections a chance to be properly closed out.
-    return new Promise(resolve => setTimeout(resolve, 200))
   }
 
   onTerminate() {

app/main.dev.js

@@ -99,18 +99,6 @@ app.on('ready', () => {
     }
   })
 
-  /**
-   * Add application event listener:
-   *  - quit app when window is closed
-   */
-  app.on('window-all-closed', () => {
-    mainLog.debug('app.window-all-closed')
-    // Respect the OSX convention of having the application in memory even after all windows have been closed
-    if (process.platform !== 'darwin') {
-      app.quit()
-    }
-  })
-
   /**
    * Add application event listener:
    *  - Stop gRPC and kill lnd process before the app windows are closed and the app quits.

test/e2e/e2e.spec.js

@@ -2,6 +2,8 @@ import { Application } from 'spectron'
 import electronPath from 'electron'
 import path from 'path'
 
+jest.unmock('electron')
+
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 15000
 
 const delay = time => new Promise(resolve => setTimeout(resolve, time))

test/unit/__mocks__/electron.js

@@ -0,0 +1,13 @@
+const { normalize } = require('path')
+
+module.exports = {
+  require: jest.fn(),
+  match: jest.fn(),
+  app: {
+    getPath: name => normalize(`/tmp/zap-test/${name}`),
+    getAppPath: () => normalize('/tmp/zap-test')
+  },
+  remote: jest.fn(),
+  dialog: jest.fn(),
+  BrowserWindow: jest.fn()
+}

test/unit/lnd/lightning.spec.js

@@ -0,0 +1,53 @@
+import { BrowserWindow } from 'electron'
+import Lightning from 'lib/lnd/lightning'
+
+jest.mock('electron-store')
+jest.mock('lib/lnd/subscribe/transactions')
+jest.mock('lib/lnd/subscribe/invoices')
+jest.mock('lib/lnd/subscribe/channelgraph')
+
+describe('Lightning', function() {
+  describe('Constructor', () => {
+    beforeAll(() => (this.lightning = new Lightning()))
+
+    describe('initial values', () => {
+      it('should set the "mainWindow" property to null', () => {
+        expect(this.lightning.mainWindow).toBeNull()
+      })
+      it('should set the "lnd" property to null', () => {
+        expect(this.lightning.lnd).toBeNull()
+      })
+      it('should initialise the "subscriptions" object with null values', () => {
+        expect(this.lightning.subscriptions).toMatchObject({
+          channelGraph: null,
+          invoices: null,
+          transactions: null
+        })
+      })
+    })
+  })
+
+  describe('subscribe()', () => {
+    beforeAll(() => {
+      this.window = new BrowserWindow({})
+      this.lightning = new Lightning()
+      this.lightning.subscribe(this.window)
+    })
+
+    it('should assign the window to the "mainWindow" property', () => {
+      expect(this.lightning.mainWindow).toBe(this.window)
+    })
+  })
+
+  describe('unsubscribe()', () => {
+    beforeAll(() => {
+      this.lightning = new Lightning()
+      this.lightning.mainWindow = new BrowserWindow({})
+      this.lightning.unsubscribe()
+    })
+
+    it('should unassign the "mainWindow" property', () => {
+      expect(this.lightning.mainWindow).toBeNull()
+    })
+  })
+})

test/unit/lnd/lnd-config.spec.js

@@ -4,46 +4,25 @@ import { join, normalize } from 'path'
 import Store from 'electron-store'
 import LndConfig from 'lib/lnd/config'
 
-jest.mock('grpc')
+jest.mock('electron-store')
-
-jest.mock('electron', () => {
-  const { normalize } = require('path')
-
-  return {
-    app: {
-      getPath: name => normalize(`/tmp/zap-test/${name}`),
-      getAppPath: () => normalize('/tmp/zap-test')
-    }
-  }
-})
-
 jest.mock('lib/lnd/util', () => {
-  const { normalize } = require('path')
-
   return {
     ...jest.requireActual('lib/lnd/util'),
-    appRootPath: normalize('/tmp/zap-test/app/root'),
     binaryName: 'binaryName',
-    binaryPath: 'binaryPath'
+    binaryPath: () => 'binaryPath'
   }
 })
 
-Store.prototype.set = jest.fn()
-
 describe('LndConfig', function() {
   const checkForStaticProperties = () => {
     it('should have "binaryPath" set to the value returned by lib/lnd/util', () => {
       expect(this.lndConfig.binaryPath).toEqual('binaryPath')
     })
     it('should have "configPath" set to "resources/lnd.conf" relative to app root from lib/lnd/util"', () => {
-      expect(this.lndConfig.configPath).toEqual(
+      expect(this.lndConfig.configPath).toEqual(normalize('/tmp/resources/lnd.conf'))
-        normalize('/tmp/zap-test/app/root/resources/lnd.conf')
-      )
     })
     it('should have "rpcProtoPath" set to "resources/rcp.proto" relative to app root from lib/lnd/util"', () => {
-      expect(this.lndConfig.rpcProtoPath).toEqual(
+      expect(this.lndConfig.rpcProtoPath).toEqual(normalize('/tmp/resources/rpc.proto'))
-        normalize('/tmp/zap-test/app/root/resources/rpc.proto')
-      )
     })
   }
 

test/unit/lnd/neutrino.spec.js

@@ -1,15 +1,6 @@
 import Neutrino from 'lib/lnd/neutrino'
 
-jest.mock('electron', () => {
+jest.mock('electron-store')
-  const { normalize } = require('path')
-
-  return {
-    app: {
-      getPath: name => normalize(`/tmp/zap-test/${name}`),
-      getAppPath: () => normalize('/tmp/zap-test')
-    }
-  }
-})
 
 describe('Neutrino', function() {
   describe('Constructor', () => {