From c97db6cd027903e5d1f638ac23c399b56da463a7 Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Mon, 9 Apr 2018 21:25:05 +0200 Subject: [PATCH] feat(lock): allow for custom lock fix: failing tests test: add a lock test suite doc: update repo docs for custom locking --- README.md | 35 ++++++++++++++++++++++-- src/index.js | 64 ++++++++++++++++++++++++++++++++++++++++---- test/lock-test.js | 63 +++++++++++++++++++++++++++++++++++++++++++ test/node.js | 37 +++++++++++++++++++++++++ test/options-test.js | 13 +++++++++ 5 files changed, 205 insertions(+), 7 deletions(-) create mode 100644 test/lock-test.js diff --git a/README.md b/README.md index 66afbcf4..c175337b 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ Arguments: * `path` (string, mandatory): the path for this repo * `options` (object, optional): may contain the following values - * `lock` (string, defaults to `"fs"` in Node.js, `"memory"` in the browser): what type of lock to use. Lock has to be acquired when opening. + * `lock` (string *Deprecated* or [Lock](#lock)), string can be `"fs"` or `"memory"`: what type of lock to use. Lock has to be acquired when opening. * `storageBackends` (object, optional): may contain the following values, which should each be a class implementing the [datastore interface](https://github.com/ipfs/interface-datastore#readme): * `root` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser). Defines the back-end type used for gets and puts of values at the root (`repo.set()`, `repo.get()`) * `blocks` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser). Defines the back-end type used for gets and puts of values at `repo.blocks`. @@ -284,7 +284,7 @@ Sets the API address. ### `repo.stat ([options], callback)` -Gets the repo status. +Gets the repo status. `options` is an object which might contain the key `human`, which is a boolean indicating whether or not the `repoSize` should be displayed in MiB or not. @@ -296,6 +296,37 @@ Gets the repo status. - `version` - `storageMax` +### Lock + +IPFS Repo comes with two built in locks: memory and fs. These can be imported via the following: + +```js +const fsLock = require('ipfs-repo/lock') // Default in Node.js +const memLock = require('ipfs-repo/lock-memory') // Default in browser +``` + +#### `lock.open (dir, callback)` + +Sets the lock if one does not already exist. + +`dir` is a string to the directory the lock should be created at. The repo typically creates the lock at its root. + +`callback` is a function with the signature `function (err, closer)`, where `closer` has a `close` method for removing the lock. + +##### `closer.close (callback)` + +Closes the lock created by `lock.open` + +`callback` is a function with the signature `function (err)`. If no error was returned, the lock was successfully removed. + +#### `lock.locked (dir, callback)` + +Checks the existence of the lock. + +`dir` is a string to the directory to check for the lock. The repo typically checks for the lock at its root. + +`callback` is a function with the signature `function (err, boolean)`, where `boolean` indicates the existence of the lock. + ## Notes - [Explanation of how repo is structured](https://github.com/ipfs/js-ipfs-repo/pull/111#issuecomment-279948247) diff --git a/src/index.js b/src/index.js index a5883c40..2a41412c 100644 --- a/src/index.js +++ b/src/index.js @@ -44,8 +44,7 @@ class IpfsRepo { this.closed = true this.path = repoPath - this._locker = lockers[this.options.lock] - assert(this._locker, 'Unknown lock type: ' + this.options.lock) + this._locker = this._getLocker() this.root = backends.create('root', this.path, this.options) this.version = version(this.root) @@ -88,7 +87,7 @@ class IpfsRepo { waterfall([ (cb) => this.root.open(ignoringAlreadyOpened(cb)), (cb) => this._isInitialized(cb), - (cb) => this._locker.lock(this.path, cb), + (cb) => this._openLock(this.path, cb), (lck, cb) => { log('aquired repo.lock') this.lockfile = lck @@ -121,7 +120,7 @@ class IpfsRepo { } ], (err) => { if (err && this.lockfile) { - this.lockfile.close((err2) => { + this._closeLock((err2) => { if (!err2) { this.lockfile = null } else { @@ -135,6 +134,61 @@ class IpfsRepo { }) } + /** + * Returns the repo locker to be used. Null will be returned if no locker is requested + * + * @private + * @returns {Locker} + */ + _getLocker () { + if (typeof this.options.lock === 'string') { + assert(lockers[this.options.lock], 'Unknown lock type: ' + this.options.lock) + return lockers[this.options.lock] + } + + assert(this.options.lock, 'No lock provided') + return this.options.lock + } + + /** + * Creates a lock on the repo if a locker is specified. The lockfile object will + * be returned in the callback if one has been created. + * + * @param {string} path + * @param {function(Error, lockfile)} callback + * @returns {void} + */ + _openLock (path, callback) { + this._locker.lock(path, callback) + } + + /** + * Closes the lock on the repo + * + * @param {function(Error)} callback + * @returns {void} + */ + _closeLock (callback) { + if (this.lockfile) { + return this.lockfile.close(callback) + } + callback() + } + + /** + * Gets the status of the lock on the repo + * + * @param {string} path + * @param {function(Error, boolean)} callback + * @returns {void} + */ + _isLocked (path, callback) { + if (this._locker) { + return this._locker.locked(path, callback) + } + callback(null, false) + } + /** * Check if the repo is already initialized. * @@ -186,7 +240,7 @@ class IpfsRepo { (cb) => { log('unlocking') this.closed = true - this.lockfile.close(cb) + this._closeLock(cb) }, (cb) => { this.lockfile = null diff --git a/test/lock-test.js b/test/lock-test.js new file mode 100644 index 00000000..670a19f3 --- /dev/null +++ b/test/lock-test.js @@ -0,0 +1,63 @@ +/* eslint-env mocha */ +'use strict' + +const chai = require('chai') +chai.use(require('dirty-chai')) +const expect = chai.expect +const series = require('async/series') +const IPFSRepo = require('../') + +module.exports = (repo) => { + describe('Repo lock tests', () => { + it('should handle locking for a repo lifecycle', (done) => { + expect(repo.lockfile).to.not.equal(null) + series([ + (cb) => { + repo.close(cb) + }, + (cb) => { + expect(repo.lockfile).to.equal(null) + cb() + }, + (cb) => { + repo.open(cb) + } + ], done) + }) + + it('should prevent multiple repos from using the same path', (done) => { + const repoClone = new IPFSRepo(repo.path, repo.options) + + // Levelup throws an uncaughtException when a lock already exists, catch it + const mochaExceptionHandler = process.listeners('uncaughtException').pop() + process.removeListener('uncaughtException', mochaExceptionHandler) + process.once('uncaughtException', function (err) { + expect(err.message).to.match(/already held/) + }) + + series([ + (cb) => { + try { + repoClone.init({}, cb) + } catch (err) { + cb(err) + } + }, + (cb) => { + repoClone.open(cb) + } + ], function (err) { + // There will be no listeners if the uncaughtException was triggered + if (process.listeners('uncaughtException').length > 0) { + expect(err.message).to.match(/already locked|already held|ENOENT/) + } + + // Reset listeners to maintain test integrity + process.removeAllListeners('uncaughtException') + process.addListener('uncaughtException', mochaExceptionHandler) + + done() + }) + }) + }) +} diff --git a/test/node.js b/test/node.js index 5bb9c323..b560fdab 100644 --- a/test/node.js +++ b/test/node.js @@ -3,6 +3,7 @@ const ncp = require('ncp').ncp const rimraf = require('rimraf') +const fs = require('fs') const path = require('path') const series = require('async/series') const chai = require('chai') @@ -13,6 +14,35 @@ const IPFSRepo = require('../src') describe('IPFS Repo Tests onNode.js', () => { require('./options-test') + const customLock = { + lockName: 'test.lock', + lock: (dir, callback) => { + customLock.locked(dir, (err, isLocked) => { + if (err || isLocked) { + return callback(new Error('already locked')) + } + + let lockPath = path.join(dir, customLock.lockName) + fs.writeFileSync(lockPath, '') + + callback(null, { + close: (cb) => { + rimraf(lockPath, cb) + } + }) + }) + }, + locked: (dir, callback) => { + fs.stat(path.join(dir, customLock.lockName), (err, stats) => { + if (err) { + callback(null, false) + } else { + callback(null, true) + } + }) + } + } + const repos = [{ name: 'default inited', opts: undefined, @@ -25,6 +55,12 @@ describe('IPFS Repo Tests onNode.js', () => { lock: 'memory' }, init: true + }, { + name: 'custom locker', + opts: { + lock: customLock + }, + init: true }, { name: 'default existing', opts: undefined, @@ -62,6 +98,7 @@ describe('IPFS Repo Tests onNode.js', () => { require('./datastore-test')(repo) require('./keystore-test')(repo) require('./stat-test')(repo) + require('./lock-test')(repo) if (!r.init) { require('./interop-test')(repo) } diff --git a/test/options-test.js b/test/options-test.js index c452fa25..09652dd2 100644 --- a/test/options-test.js +++ b/test/options-test.js @@ -28,6 +28,19 @@ describe('custom options tests', () => { const repo = new Repo(repoPath) expect(repo.options).to.deep.equal(expectedRepoOptions()) }) + + it('allows for a custom locker', () => { + const lock = { + lock: (path, callback) => { }, + locked: (path, callback) => { } + } + + const repo = new Repo(repoPath, { + lock + }) + + expect(repo._getLocker()).to.deep.equal(lock) + }) }) function noop () {}