From 9f4712f1f875d4d6f6fb37bd6bed85b4fee040b7 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Thu, 24 Aug 2017 17:24:48 -0400 Subject: [PATCH] refactor: use an SDK orchestrator to implement drive scanning (#1707) This is a major first step towards adopting an SDK architecture. This commit creates an SDK adaptor with a `.scan()` function that uses `drivelist` under the hood. Then, an SDK orchestrator is used to provide drive scanning capabilities to the GUI. Here's a list of some particularly interesting changes: - The drives returned by the SDK adaptor now have a "pending" and an "adaptor" property. The "pending" property is a boolean flag that determines if the drive is ready to be used (this will come handy for usbboot), while the "adaptor" property simply contains the name of the adaptor that drive came from - The GUI drive scanner Rx implementation was replaces with a "promise loop." Before, the drive scanning routine would be called every 2 seconds (without waiting for the previous scan to complete), while now, the next scan happens *after* the previous scan completes. For this reason, I reduced the drive scanning interval timeout to match the timing we had before Change-Type: patch See: https://github.com/resin-io/etcher/pull/1686 Signed-off-by: Juan Cruz Viotti --- lib/gui/modules/drive-scanner.js | 100 +++++++++++++++--------- lib/shared/sdk/index.js | 58 ++++++++++++++ lib/shared/sdk/standard/index.js | 56 +++++++++++++ npm-shrinkwrap.json | 5 -- package.json | 1 - tests/gui/modules/drive-scanner.spec.js | 72 +++++++++++++---- 6 files changed, 235 insertions(+), 57 deletions(-) create mode 100644 lib/shared/sdk/index.js create mode 100644 lib/shared/sdk/standard/index.js diff --git a/lib/gui/modules/drive-scanner.js b/lib/gui/modules/drive-scanner.js index 2dc8d242..7254c5a1 100644 --- a/lib/gui/modules/drive-scanner.js +++ b/lib/gui/modules/drive-scanner.js @@ -16,40 +16,25 @@ 'use strict' -const Rx = require('rx') -const _ = require('lodash') const EventEmitter = require('events').EventEmitter -const drivelist = require('drivelist') +const Bluebird = require('bluebird') const settings = require('../models/settings') +const sdk = require('../../shared/sdk') -const DRIVE_SCANNER_INTERVAL_MS = 2000 -const DRIVE_SCANNER_FIRST_SCAN_DELAY_MS = 0 +/** + * @summary Time to wait between scans + * @type {Number} + * @constant + */ +const DRIVE_SCANNER_INTERVAL_MS = 1000 + +/** + * @summary Scanner event emitter singleton instance + * @type {Object} + * @constant + */ const emitter = new EventEmitter() -/* eslint-disable lodash/prefer-lodash-method */ - -const availableDrives = Rx.Observable.timer( - DRIVE_SCANNER_FIRST_SCAN_DELAY_MS, - DRIVE_SCANNER_INTERVAL_MS -) - -/* eslint-enable lodash/prefer-lodash-method */ - - .flatMap(() => { - return Rx.Observable.fromNodeCallback(drivelist.list)() - }) - - .map((drives) => { - if (settings.get('unsafeMode')) { - return drives - } - - return _.reject(drives, { - system: true - }) - }) - .pausable(new Rx.Subject()) - /* * This service emits the following events: * @@ -68,11 +53,51 @@ const availableDrives = Rx.Observable.timer( * }); * ``` */ -availableDrives.subscribe((drives) => { - emitter.emit('drives', drives) -}, (error) => { - emitter.emit('error', error) -}) + +/** + * @summary Flag to control scanning status + * @type {Boolean} + */ +let scanning = false + +/** + * @summary Start the scanning loop + * @function + * @private + * + * @description + * This function emits `drives` or `error` events + * using the event emitter singleton instance. + * + * @returns {Promise} + * + * @example + * scanning = true + * scan() + */ +const scan = () => { + if (!scanning) { + return Bluebird.resolve() + } + + return sdk.scan({ + standard: { + includeSystemDrives: settings.get('unsafeMode') + } + }).then((drives) => { + emitter.emit('drives', drives) + }).catch((error) => { + emitter.emit('error', error) + }).finally(() => { + if (!scanning) { + return Bluebird.resolve() + } + + return Bluebird + .delay(DRIVE_SCANNER_INTERVAL_MS) + .then(scan) + }) +} /** * @summary Start scanning drives @@ -83,7 +108,10 @@ availableDrives.subscribe((drives) => { * driveScanner.start(); */ emitter.start = () => { - availableDrives.resume() + if (!scanning) { + scanning = true + scan() + } } /** @@ -95,7 +123,7 @@ emitter.start = () => { * driveScanner.stop(); */ emitter.stop = () => { - availableDrives.pause() + scanning = false } module.exports = emitter diff --git a/lib/shared/sdk/index.js b/lib/shared/sdk/index.js new file mode 100644 index 00000000..10e1f0b9 --- /dev/null +++ b/lib/shared/sdk/index.js @@ -0,0 +1,58 @@ +/* + * Copyright 2017 resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict' + +const Bluebird = require('bluebird') +const _ = require('lodash') + +/** + * @summary The list of loaded adaptors + * @type {Object[]} + * @constant + */ +const ADAPTORS = [ + require('./standard') +] + +/** + * @summary Scan for drives using all registered adaptors + * @function + * @public + * + * @description + * The options object contains options for all the registered + * adaptors. For the `standard` adaptor, for example, place + * options in `options.standard`. + * + * @param {Object} options - options + * @fulfil {Object[]} - drives + * @returns {Promise} + * + * @example + * sdk.scan({ + * standard: { + * includeSystemDrives: true + * } + * }).then((drives) => { + * console.log(drives) + * }) + */ +exports.scan = (options) => { + return Bluebird.all(_.map(ADAPTORS, (adaptor) => { + return adaptor.scan(_.get(options, [ adaptor.name ], {})) + })).then(_.flatten) +} diff --git a/lib/shared/sdk/standard/index.js b/lib/shared/sdk/standard/index.js new file mode 100644 index 00000000..bcc5be6e --- /dev/null +++ b/lib/shared/sdk/standard/index.js @@ -0,0 +1,56 @@ +/* + * Copyright 2017 resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict' + +const Bluebird = require('bluebird') +const drivelist = Bluebird.promisifyAll(require('drivelist')) + +/** + * @summary The name of this adaptor + * @public + * @type {String} + * @constant + */ +exports.name = 'standard' + +/** + * @summary Scan for block devices + * @function + * @public + * + * @param {Object} [options] - options + * @param {Object} [options.includeSystemDrives=false] - include system drives + * @fulfil {Object[]} - block devices + * @returns {Promise} + * + * @example + * standard.scan({ + * includeSystemDrives: true + * }).each((device) => { + * console.log(device) + * }) + */ +exports.scan = (options = {}) => { + // eslint-disable-next-line lodash/prefer-lodash-method + return drivelist.listAsync().filter((drive) => { + return options.includeSystemDrives || !drive.system + }).map((drive) => { + drive.pending = false + drive.adaptor = exports.name + return drive + }) +} diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index e1018d90..30c10244 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -8995,11 +8995,6 @@ "from": "run-async@>=0.1.0 <0.2.0", "resolved": "https://registry.npmjs.org/run-async/-/run-async-0.1.0.tgz" }, - "rx": { - "version": "4.1.0", - "from": "rx@4.1.0", - "resolved": "https://registry.npmjs.org/rx/-/rx-4.1.0.tgz" - }, "rx-lite": { "version": "3.1.2", "from": "rx-lite@>=3.1.2 <4.0.0", diff --git a/package.json b/package.json index 37841674..a45da05b 100644 --- a/package.json +++ b/package.json @@ -77,7 +77,6 @@ "resin-cli-form": "1.4.1", "resin-cli-visuals": "1.3.1", "resin-corvus": "1.0.0-beta.28", - "rx": "4.1.0", "semver": "5.1.1", "sudo-prompt": "6.1.0", "trackjs": "2.3.1", diff --git a/tests/gui/modules/drive-scanner.spec.js b/tests/gui/modules/drive-scanner.spec.js index 38fc8a20..65647a6b 100644 --- a/tests/gui/modules/drive-scanner.spec.js +++ b/tests/gui/modules/drive-scanner.spec.js @@ -18,27 +18,34 @@ const m = require('mochainon') const os = require('os') +const Bluebird = require('bluebird') const drivelist = require('drivelist') const driveScanner = require('../../../lib/gui/modules/drive-scanner') +const sdk = require('../../../lib/shared/sdk') describe('Browser: driveScanner', function () { describe('given no available drives', function () { beforeEach(function () { - this.drivelistStub = m.sinon.stub(drivelist, 'list') - this.drivelistStub.yields(null, []) + this.sdkScanStub = m.sinon.stub(sdk, 'scan') + this.sdkScanStub.returns(Bluebird.resolve([])) }) afterEach(function () { - this.drivelistStub.restore() + this.sdkScanStub.restore() }) it('should emit an empty array', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.deep.equal([]) + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -46,17 +53,19 @@ describe('Browser: driveScanner', function () { describe('given only system available drives', function () { beforeEach(function () { this.drivelistStub = m.sinon.stub(drivelist, 'list') - this.drivelistStub.yields(null, [ { - device: '/dev/sda', - description: 'WDC WD10JPVX-75J', - size: '931.5G', - mountpoints: [ - { - path: '/' - } - ], - system: true - } ]) + this.drivelistStub.yields(null, [ + { + device: '/dev/sda', + description: 'WDC WD10JPVX-75J', + size: '931.5G', + mountpoints: [ + { + path: '/' + } + ], + system: true + } + ]) }) afterEach(function () { @@ -64,12 +73,17 @@ describe('Browser: driveScanner', function () { }) it('should emit an empty array', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.deep.equal([]) + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -132,6 +146,8 @@ describe('Browser: driveScanner', function () { }) it('should emit the non removable drives', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.deep.equal([ { @@ -144,6 +160,8 @@ describe('Browser: driveScanner', function () { path: '/mnt/foo' } ], + pending: false, + adaptor: 'standard', system: false }, { @@ -156,14 +174,19 @@ describe('Browser: driveScanner', function () { path: '/mnt/bar' } ], + pending: false, + adaptor: 'standard', system: false } ]) + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -223,6 +246,8 @@ describe('Browser: driveScanner', function () { }) it('should emit the non removable drives', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.deep.equal([ { @@ -231,6 +256,8 @@ describe('Browser: driveScanner', function () { description: 'Foo', size: '14G', mountpoints: [], + pending: false, + adaptor: 'standard', system: false }, { @@ -243,14 +270,19 @@ describe('Browser: driveScanner', function () { path: 'F:' } ], + pending: false, + adaptor: 'standard', system: false } ]) + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -279,13 +311,18 @@ describe('Browser: driveScanner', function () { }) it('should use the drive letter as the name', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.have.length(1) m.chai.expect(drives[0].displayName).to.equal('F:') + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -320,13 +357,18 @@ describe('Browser: driveScanner', function () { }) it('should join all the mountpoints in `name`', function (done) { + const spy = m.sinon.spy() + driveScanner.once('drives', function (drives) { m.chai.expect(drives).to.have.length(1) m.chai.expect(drives[0].displayName).to.equal('F:, G:, H:') + m.chai.expect(spy).to.not.have.been.called + driveScanner.removeListener('error', spy) driveScanner.stop() done() }) + driveScanner.on('error', spy) driveScanner.start() }) }) @@ -343,7 +385,7 @@ describe('Browser: driveScanner', function () { }) it('should emit the error', function (done) { - driveScanner.on('error', function (error) { + driveScanner.once('error', function (error) { m.chai.expect(error).to.be.an.instanceof(Error) m.chai.expect(error.message).to.equal('scan error') driveScanner.stop()