From a16fdec23b455995fa5ab53ab3a57cea2cd8742e Mon Sep 17 00:00:00 2001 From: Tom Kirkpatrick Date: Mon, 27 Aug 2018 10:27:06 +0200 Subject: [PATCH 1/2] fix(lnd): ensure graceful shutdown Lnd doesn't currently do a graceful shutdown when a `SIGTERM` is issued. To ensure a graceful shutdown, call `stopDaemon` on the gRPC interface prior to terminating lnd. If a graceful shutdown is unsuccessful then force kill the daemon. --- app/lib/lnd/lightning.js | 69 +++++++++++++-- app/lib/lnd/neutrino.js | 8 +- app/lib/zap/controller.js | 148 ++++++++++++++++++++------------- test/unit/lnd/neutrino.spec.js | 4 +- 4 files changed, 159 insertions(+), 70 deletions(-) diff --git a/app/lib/lnd/lightning.js b/app/lib/lnd/lightning.js index 81e466e3..170a3816 100644 --- a/app/lib/lnd/lightning.js +++ b/app/lib/lnd/lightning.js @@ -3,6 +3,7 @@ import grpc from 'grpc' import { loadSync } from '@grpc/proto-loader' import { BrowserWindow } from 'electron' +import StateMachine from 'javascript-state-machine' import LndConfig from './config' import { getDeadline, validateHost, createSslCreds, createMacaroonCreds } from './util' import methods from './methods' @@ -25,24 +26,43 @@ type LightningSubscriptionsType = { class Lightning { mainWindow: BrowserWindow lnd: any + lndConfig: LndConfig subscriptions: LightningSubscriptionsType + _fsm: StateMachine - constructor() { + // Transitions provided by the state machine. + connect: any + disconnect: any + terminate: any + is: any + can: any + state: string + + constructor(lndConfig: LndConfig) { this.mainWindow = null this.lnd = null + this.lndConfig = lndConfig this.subscriptions = { channelGraph: null, invoices: null, transactions: null } + + // Initialize the state machine. + this._fsm() } + // ------------------------------------ + // FSM Callbacks + // ------------------------------------ + /** * Connect to the gRPC interface and verify it is functional. * @return {Promise} */ - async connect(lndConfig: LndConfig) { - const { rpcProtoPath, host, cert, macaroon } = lndConfig + async onBeforeConnect() { + mainLog.info('Connecting to Lightning gRPC service') + const { rpcProtoPath, host, cert, macaroon } = this.lndConfig // Verify that the host is valid before creating a gRPC client that is connected to it. return await validateHost(host).then(async () => { @@ -69,16 +89,15 @@ class Lightning { const credentials = grpc.credentials.combineChannelCredentials(sslCreds, macaroonCreds) // Create a new gRPC client instance. - const lnd = new rpc.lnrpc.Lightning(host, credentials) + this.lnd = new rpc.lnrpc.Lightning(host, credentials) - // Call the getInfo method to ensure that we can make successful calls to the gRPC interface. + // Wait for the gRPC connection to be established. return new Promise((resolve, reject) => { - lnd.getInfo({}, { deadline: getDeadline(2) }, err => { + grpc.waitForClientReady(this.lnd, getDeadline(2), err => { if (err) { return reject(err) } - this.lnd = lnd - return resolve(lnd) + return resolve() }) }) }) @@ -87,13 +106,34 @@ class Lightning { /** * Discomnnect the gRPC service. */ - disconnect() { + onBeforeDisconnect() { + mainLog.info('Disconnecting from Lightning gRPC service') this.unsubscribe() if (this.lnd) { this.lnd.close() } } + /** + * Gracefully shutdown the gRPC service. + */ + async onBeforeTerminate() { + mainLog.info('Shutting down Lightning daemon') + this.unsubscribe() + return new Promise((resolve, reject) => { + this.lnd.stopDaemon({}, (err, data) => { + if (err) { + return reject(err) + } + resolve(data) + }) + }) + } + + // ------------------------------------ + // Helpers + // ------------------------------------ + /** * Hook up lnd restful methods. */ @@ -105,6 +145,7 @@ class Lightning { * Subscribe to all bi-directional streams. */ subscribe(mainWindow: BrowserWindow) { + mainLog.info('Subscribing to Lightning gRPC streams') this.mainWindow = mainWindow this.subscriptions.channelGraph = subscribeToChannelGraph.call(this) @@ -116,6 +157,7 @@ class Lightning { * Unsubscribe from all bi-directional streams. */ unsubscribe() { + mainLog.info('Unsubscribing from Lightning gRPC streams') this.mainWindow = null Object.keys(this.subscriptions).forEach(subscription => { if (this.subscriptions[subscription]) { @@ -126,4 +168,13 @@ class Lightning { } } +StateMachine.factory(Lightning, { + init: 'ready', + transitions: [ + { name: 'connect', from: 'ready', to: 'connected' }, + { name: 'disconnect', from: 'connected', to: 'ready' }, + { name: 'terminate', from: 'connected', to: 'ready' } + ] +}) + export default Lightning diff --git a/app/lib/lnd/neutrino.js b/app/lib/lnd/neutrino.js index 65299999..23914b07 100644 --- a/app/lib/lnd/neutrino.js +++ b/app/lib/lnd/neutrino.js @@ -141,7 +141,10 @@ class Neutrino extends EventEmitter { if (this.is(CHAIN_SYNC_PENDING) || this.is(CHAIN_SYNC_IN_PROGRESS)) { // If we cant get a connectionn to the backend. - if (line.includes('Waiting for chain backend to finish sync')) { + if ( + line.includes('Waiting for chain backend to finish sync') || + line.includes('Waiting for block headers to sync, then will start cfheaders sync') + ) { this.setState(CHAIN_SYNC_WAITING) } // If we are still waiting for the back end to finish synncing. @@ -223,8 +226,9 @@ class Neutrino extends EventEmitter { /** * Stop the Lnd process. */ - stop() { + kill() { if (this.process) { + mainLog.info('Killing Neutrino process...') this.process.kill() this.process = null } diff --git a/app/lib/zap/controller.js b/app/lib/zap/controller.js index 25e12bc7..794f867a 100644 --- a/app/lib/zap/controller.js +++ b/app/lib/zap/controller.js @@ -51,10 +51,9 @@ const grpcSslCipherSuites = connectionType => */ class ZapController { mainWindow: BrowserWindow - neutrino: any - lightning: any + neutrino: Neutrino + lightning: Lightning splashScreenTime: number - lightningGrpcConnected: boolean lndConfig: LndConfig _fsm: StateMachine @@ -73,18 +72,9 @@ class ZapController { // Variable to hold the main window instance. this.mainWindow = mainWindow - // Keep a reference any neutrino process started by us. - this.neutrino = undefined - - // Keep a reference to the lightning gRPC instance. - this.lightning = undefined - // Time for the splash screen to remain visible. this.splashScreenTime = 500 - // Boolean indicating wether the lightning grpc is connected ot not. - this.lightningGrpcConnected = false - // Initialize the state machine. this._fsm() @@ -127,7 +117,7 @@ class ZapController { // FSM Callbacks // ------------------------------------ - onOnboarding() { + async onOnboarding(lifecycle: any) { mainLog.debug('[FSM] onOnboarding...') // Remove any existing IPC listeners so that we can start fresh. @@ -136,12 +126,14 @@ class ZapController { // Register IPC listeners so that we can react to instructions coming from the app. this._registerIpcListeners() - // Ensure wallet is disconnected. - this.disconnectLightningWallet() + // Disconnect any pre-existing lightning wallet connection. + if (lifecycle.from === 'connected' && this.lightning && this.lightning.can('disconnect')) { + this.lightning.disconnect() + } - // If Neutrino is running, kill it. - if (this.neutrino) { - this.neutrino.stop() + // If we are comming from a running state, stop the Neutrino process. + else if (lifecycle.from === 'running') { + await this.shutdownNeutrino() } // Give the grpc connections a chance to be properly closed out. @@ -209,15 +201,16 @@ class ZapController { }) } - onTerminated() { + async onTerminated(lifecycle: any) { mainLog.debug('[FSM] onTerminated...') - // Unsubscribe the gRPC streams before thhe window closes. This ensures that we can properly reestablish a fresh - // connection when a new window is opened. - this.disconnectLightningWallet() - // If Neutrino is running, kill it. - if (this.neutrino) { - this.neutrino.stop() + // Disconnect from any existing lightning wallet connection. + if (lifecycle.from === 'connected' && this.lightning && this.lightning.can('disconnect')) { + this.lightning.disconnect() + } + // If we are comming from a running state, stop the Neutrino process. + else if (lifecycle.from === 'running') { + await this.shutdownNeutrino() } } @@ -251,7 +244,7 @@ class ZapController { * Start the wallet unlocker. */ startWalletUnlocker() { - mainLog.info('Starting wallet unlocker...') + mainLog.info('Establishing connection to Wallet Unlocker gRPC interface...') try { const walletUnlockerMethods = initWalletUnlocker(this.lndConfig) @@ -275,42 +268,23 @@ class ZapController { * Create and subscribe to the Lightning service. */ async startLightningWallet() { - if (this.lightningGrpcConnected) { - return - } - mainLog.info('Starting lightning wallet...') - this.lightning = new Lightning() + mainLog.info('Establishing connection to Lightning gRPC interface...') + this.lightning = new Lightning(this.lndConfig) // Connect to the Lightning interface. - await this.lightning.connect(this.lndConfig) - - // Subscribe the main window to receive streams. - this.lightning.subscribe(this.mainWindow) - - // Listen for all gRPC restful methods and pass to gRPC. - ipcMain.on('lnd', (event, { msg, data }) => this.lightning.lndMethods(event, msg, data)) + try { + await this.lightning.connect() - // Let the renderer know that we are connected. - this.sendMessage('lightningGrpcActive') + this.lightning.subscribe(this.mainWindow) - // Update our internal state. - this.lightningGrpcConnected = true - } + // Listen for all gRPC restful methods and pass to gRPC. + ipcMain.on('lnd', (event, { msg, data }) => this.lightning.lndMethods(event, msg, data)) - /** - * Unsubscribe from the Lightning service. - */ - disconnectLightningWallet() { - if (!this.lightningGrpcConnected) { - return + // Let the renderer know that we are connected. + this.sendMessage('lightningGrpcActive') + } catch (err) { + mainLog.warn('Unable to connect to Lighitnng gRPC interface: %o', err) } - mainLog.info('Disconnecting lightning Wallet...') - - // Disconnect streams. - this.lightning.disconnect() - - // Update our internal state. - this.lightningGrpcConnected = false } /** @@ -337,7 +311,7 @@ class ZapController { if (this.is('running') || this.is('connected')) { dialog.showMessageBox({ type: 'error', - message: `Lnd has unexpectadly quit: ${lastError}` + message: `Lnd has unexpectedly quit: ${lastError}` }) this.terminate() } @@ -383,6 +357,66 @@ class ZapController { this.neutrino.start() } + /** + * Gracefully shutdown LND. + */ + async shutdownNeutrino() { + // We only want to shut down LND if we are running it locally. + if (this.lndConfig.type !== 'local') { + return Promise.resolve() + } + + // Attempt a graceful shutdown if we can. + if (this.lightning && this.lightning.can('terminate')) { + mainLog.info('Shutting down Neutrino...') + + return new Promise(resolve => { + // HACK: Sometimes there are errors during the shutdown process that prevent the daeming from shutting down at + // all. If we haven't received notification of the process closing within 10 seconds, kill it. + // See https://github.com/lightningnetwork/lnd/pull/1781 + // See https://github.com/lightningnetwork/lnd/pull/1783 + const shutdownTimeout = setTimeout(() => { + this.neutrino.removeListener('close', closeHandler) + if (this.neutrino) { + mainLog.warn('Graceful shutdown failed to complete within 30 seconds.') + this.neutrino.kill() + resolve() + } + }, 1000 * 10) + + // HACK: The Lightning.stopDaemon` call returns before lnd has actually fully completed the shutdown process + // so we add a listener on the close event so that we can wrap things up once the process has been fully closed + // out. + const closeHandler = function() { + mainLog.info('Neutrino shutdown complete.') + clearTimeout(shutdownTimeout) + resolve() + } + this.neutrino.once('close', closeHandler) + + this.lightning + .terminate() + .then(() => mainLog.info('Neutrino Daemon shutdown complete')) + .catch(err => { + mainLog.error('Unable to gracefully shutdown LND: %o', err) + // Kill the process ourselves here to ensure that we don't leave hanging processes. + if (this.neutrino) { + this.neutrino.kill() + resolve() + } + }) + }) + } + + // The Lightning service is only active after the wallet has been unlocked and a gRPC connection has been + // established. In this case, kill the Neutrino process to ensure that we don't leave hanging process. + // FIXME: This currencly doesn't do a graceful shutdown as LND does not properly handle SIGTERM. + // See https://github.com/lightningnetwork/lnd/issues/1028 + else if (this.neutrino) { + this.neutrino.kill() + } + } + finishOnboarding(options: onboardingOptions) { mainLog.info('Finishing onboarding') // Save the lnd config options that we got from the renderer. diff --git a/test/unit/lnd/neutrino.spec.js b/test/unit/lnd/neutrino.spec.js index 71827a27..2b7dcaba 100644 --- a/test/unit/lnd/neutrino.spec.js +++ b/test/unit/lnd/neutrino.spec.js @@ -177,14 +177,14 @@ describe('Neutrino', function() { }) }) - describe('.stop', () => { + describe('.kill', () => { describe('called when neutrino is already running', () => { beforeEach(() => { this.neutrino = new Neutrino() this.neutrino.process = { kill: jest.fn() } - this.neutrino.stop() + this.neutrino.kill() }) it('should kill the neutrino process', () => { expect(this.neutrino.process).toBeNull() From 133e34dea2a9bce630e2c0f07f902a06576263dc Mon Sep 17 00:00:00 2001 From: Tom Kirkpatrick Date: Mon, 27 Aug 2018 16:02:32 +0200 Subject: [PATCH 2/2] test(neutrino): basic neutrino process spawn test --- package.json | 1 + test/unit/lnd/neutrino.spec.js | 46 ++++++++++++++++++++++++++-------- yarn.lock | 8 +++++- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/package.json b/package.json index 6288f37b..097e1564 100644 --- a/package.json +++ b/package.json @@ -258,6 +258,7 @@ "lint-staged": "^7.2.0", "lnd-binary": "^0.3.5", "minimist": "^1.2.0", + "mock-spawn": "^0.2.6", "node-sass": "^4.9.0", "prettier": "^1.13.5", "react-addons-test-utils": "^15.6.2", diff --git a/test/unit/lnd/neutrino.spec.js b/test/unit/lnd/neutrino.spec.js index 2b7dcaba..7c645ee6 100644 --- a/test/unit/lnd/neutrino.spec.js +++ b/test/unit/lnd/neutrino.spec.js @@ -1,10 +1,22 @@ +// @flow + import Neutrino from 'lib/lnd/neutrino' +import LndConfig from 'lib/lnd/config' +import mockSpawn from 'mock-spawn' jest.mock('electron-store') +jest.mock('child_process', () => { + var mockSpawn = require('mock-spawn') + return { + spawn: mockSpawn() + } +}) describe('Neutrino', function() { describe('Constructor', () => { - beforeAll(() => (this.neutrino = new Neutrino())) + beforeAll(() => { + this.neutrino = new Neutrino(new LndConfig()) + }) describe('initial values', () => { it('should set the "process" property to null', () => { @@ -37,7 +49,7 @@ describe('Neutrino', function() { describe('.setState', () => { describe('called with new state', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = 'chain-sync-finished' this.neutrino.on('chain-sync-finished', this.callback) @@ -53,7 +65,7 @@ describe('Neutrino', function() { }) describe('called with current state', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = 'chain-sync-pending' this.neutrino.on('chain-sync-pending', this.callback) @@ -72,7 +84,7 @@ describe('Neutrino', function() { describe('.setCurrentBlockHeight', () => { describe('called with higher height', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = 100 this.neutrino.on('got-current-block-height', this.callback) @@ -89,7 +101,7 @@ describe('Neutrino', function() { }) describe('called with lower height', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = -1 this.neutrino.on('got-current-block-height', this.callback) @@ -108,7 +120,7 @@ describe('Neutrino', function() { describe('.setLndBlockHeight', () => { describe('called with higher height', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = 100 this.neutrino.on('got-lnd-block-height', this.callback) @@ -130,7 +142,7 @@ describe('Neutrino', function() { }) describe('called with lower height', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.callback = jest.fn() this.newVal = -1 this.neutrino.on('got-lnd-block-height', this.callback) @@ -152,7 +164,9 @@ describe('Neutrino', function() { describe('.is', () => { describe('called with current state', () => { - beforeEach(() => (this.neutrino = new Neutrino())) + beforeEach(() => { + this.neutrino = new Neutrino(new LndConfig()) + }) it('should returnn true if the current state matches', () => { expect(this.neutrino.is('chain-sync-pending')).toEqual(true) @@ -164,10 +178,20 @@ describe('Neutrino', function() { }) describe('.start', () => { + describe('called when neutrino is not running', () => { + beforeEach(() => { + this.neutrino = new Neutrino(new LndConfig()) + this.neutrino.start() + }) + it('should set the subprocess object on the `process` property', () => { + expect(this.neutrino.process.pid).toBeDefined() + }) + }) + describe('called when neutrino is already running', () => { beforeEach(() => { - this.neutrino = new Neutrino() - this.neutrino.process = 123 + this.neutrino = new Neutrino(new LndConfig()) + this.neutrino.process = mockSpawn() }) it('should throw an error', () => { expect(() => { @@ -180,7 +204,7 @@ describe('Neutrino', function() { describe('.kill', () => { describe('called when neutrino is already running', () => { beforeEach(() => { - this.neutrino = new Neutrino() + this.neutrino = new Neutrino(new LndConfig()) this.neutrino.process = { kill: jest.fn() } diff --git a/yarn.lock b/yarn.lock index ea73a0a3..39fd055c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8084,6 +8084,12 @@ mkdirp@0.5.0: dependencies: minimist "0.0.8" +mock-spawn@^0.2.6: + version "0.2.6" + resolved "https://registry.yarnpkg.com/mock-spawn/-/mock-spawn-0.2.6.tgz#b39c15a1c067504310144151f2c1de344d03937f" + dependencies: + through "2.3.x" + moment@^2.22.2: version "2.22.2" resolved "https://registry.yarnpkg.com/moment/-/moment-2.22.2.tgz#3c257f9839fc0e93ff53149632239eb90783ff66" @@ -11648,7 +11654,7 @@ through2@~0.2.3: readable-stream "~1.1.9" xtend "~2.1.1" -through@2, "through@>=2.2.7 <3", through@^2.3.6, through@^2.3.8: +through@2, through@2.3.x, "through@>=2.2.7 <3", through@^2.3.6, through@^2.3.8: version "2.3.8" resolved "https://registry.yarnpkg.com/through/-/through-2.3.8.tgz#0dd4c9ffaabc357960b1b724115d7e0e86a2e1f5"