feat(lock): allow for custom lock options #162

jacobheun · 2018-04-09T19:28:52Z

Allows users to create custom repo locks, such as to support the new s3 datastore
~~Allows users to have no repo locking~~

resolves #161

daviddias · 2018-04-09T23:57:28Z

@pgte @dignifiedquire can I get a review from you on this feature?

daviddias

@jacobheun please add docs about this feature

dignifiedquire · 2018-04-10T13:53:34Z

test/options-test.js

+
+  it('allows for no locker', () => {
+    const repo = new Repo(repoPath, {
+      locker: false


should also test for undefined options

dignifiedquire · 2018-04-10T13:54:56Z

test/node.js

@@ -25,6 +41,16 @@ describe('IPFS Repo Tests onNode.js', () => {
      lock: 'memory'
    },
    init: true
+  }, {
+    name: 'custom locker',


There is no proper locking tests currently, so it would be good to test various locking scenarios in addition to the regular tests

dignifiedquire · 2018-04-10T13:56:04Z

src/index.js

+   * @returns {Locker}
+   */
+  _getLocker () {
+    if (this.options.locker === false) {


This way of options is 👎 from my side. This forces locker to be of multiple types, so disabling the locker should check for != null and be if it is not set, rather than a boolean

I know multiple types for an option isn’t especially desirable, but it feels to me like just using lock for both custom and built-in types instead of adding a new option would be much nicer from the perspective of a user. Maybe we could move towards the type being a Locker object instead of a string? You could support built-ins with:

// Expose the built-in collection as a frozen static var on Repo new Repo({ lock: Repo.lockers.memory }) // OR new Repo({ lock: require('ipfs-repo/lock-memory') })

…though we’d probably want to keep string support as a deprecated feature for a little while.

(Found my way here after reviewing ipfs/js-ipfs#1303; I hope I’m not butting in.)

dignifiedquire · 2018-04-10T13:57:29Z

src/index.js

+   * @returns {void}
+   */
+  _openLock (path, callback) {
+    if (this._locker) {


also 👎 on being able to disable the locking, this is just asking for trouble

I am fine with leaving locking required, the custom locker is the needed piece. It's fairly easy to subvert locking at the moment even if we don't explicitly allow it to be disabled. With custom datastore support there is a potential use case for users wanting to handle locking at the object level, rather than the repo level.

If we decide to leave it required, I can update the documentation when I add in the custom locker docs to encourage users to use locking appropriate for their use case and custom datastores, such as storing their lock on S3 if their datastore is there.

fix: failing tests test: add a lock test suite doc: update repo docs for custom locking

jacobheun · 2018-04-16T23:50:00Z

@dignifiedquire @diasdavid

I've made a few updates after everyones feedback:

Documentation has been added to the Readme to help users see how to add a custom lock
I have removed the ability to disable locking, so this feature update can focus on the customization aspect only. This should give users the ability to add appropriate locking based on their datastore types.
I've added a suite to test that locks are being applied and that locking enforces one Node per Repo.

I also updated the repo options to allow for overloading of the lock option as a string or Lock, instead of a separate property. As @Mr0grog mentioned, I also think this is more friendly for users. I noted that specifying lock as a string is Deprecated, but it is still currently supported to allow for backwards compatibility.

dignifiedquire

thanks, looks much better now

Mr0grog

Added some quick thoughts on the docs. Looks good!

Mr0grog · 2018-04-17T15:56:48Z

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.


I think it’s generally clearer (and helps encourage correct usage) to describe the intended or standard usage first, then what’s deprecated or not desired — so I think it might be better to describe using an actual Lock type first, then describe the deprecated string support.

Mr0grog · 2018-04-17T15:59:01Z

README.md

+
+```js
+const fsLock = require('ipfs-repo/src/lock')  // Default in Node.js
+const memLock = require('ipfs-repo/src/lock-memory')  // Default in browser


When it comes to examples and documentation, where a reader might not be familiar with all the concepts or names being used, it helps to avoid abbreviation if you can (so memoryLock might be better than memLock here).

Mr0grog · 2018-04-17T15:59:24Z

README.md

+const memLock = require('ipfs-repo/src/lock-memory')  // Default in browser
+```
+
+#### `lock.open (dir, callback)`


Isn’t this lock.lock (directory, callback)?

Also, it might help to preface this section with a sentence that says:

You can also provide your own custom Lock. It must be an object with the following interface:

Mr0grog · 2018-04-17T16:02:27Z

README.md

+
+#### `lock.open (dir, callback)`
+
+Sets the lock if one does not already exist.


It should also call callback(error) with an error if a lock does exist, right?

Mr0grog · 2018-04-17T16:18:28Z

src/index.js

+   * @returns {void}
+   */
+  _openLock (path, callback) {
+    this._locker.lock(path, callback)


Since this can be user-provided now, should we validate that the resulting object has a close() method? (If yes, we probably also need to change _closeLock so it doesn’t try and close the unclose-able lock when handling the error 😛)

Ensure locks have a close method when creating the lock

jacobheun · 2018-04-17T19:05:57Z

@Mr0grog I updated things based on your comments, and I agree we should check for the lock.close method. I updated the code and added a test to verify it. If the assertion error is triggered, the internal lockfile reference won't be set, so _closeLock won't get called.

daviddias

Looking HAWT 🔥

ghost assigned jacobheun Apr 9, 2018

ghost added the status/in-progress In progress label Apr 9, 2018

jacobheun force-pushed the feat/locker branch from bd91d54 to 9058d97 Compare April 9, 2018 19:42

jacobheun mentioned this pull request Apr 9, 2018

Create an example for configuring the ipfs repo ipfs/js-ipfs#1303

Merged

daviddias requested review from pgte and dignifiedquire April 9, 2018 23:56

daviddias requested changes Apr 9, 2018

View reviewed changes

dignifiedquire suggested changes Apr 10, 2018

View reviewed changes

feat(lock): allow for custom lock

1b83f6c

fix: failing tests test: add a lock test suite doc: update repo docs for custom locking

jacobheun force-pushed the feat/locker branch from 9058d97 to 1b83f6c Compare April 16, 2018 20:57

docs: fix docs for lock include

87abd7c

jacobheun changed the title ~~feat(lock): allow for custom lock and no lock options~~ feat(lock): allow for custom lock options Apr 16, 2018

fix: fix tests for ci

d844eba

dignifiedquire previously approved these changes Apr 17, 2018

View reviewed changes

Mr0grog reviewed Apr 17, 2018

View reviewed changes

docs: update docs to be clearer

728c644

Ensure locks have a close method when creating the lock

jacobheun dismissed dignifiedquire’s stale review via 728c644 April 17, 2018 19:02

daviddias approved these changes Apr 23, 2018

View reviewed changes

daviddias merged commit 460dafb into master Apr 23, 2018

daviddias deleted the feat/locker branch April 23, 2018 08:47

ghost removed the status/in-progress In progress label Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lock): allow for custom lock options #162

feat(lock): allow for custom lock options #162

jacobheun commented Apr 9, 2018 •

edited

Loading

daviddias commented Apr 9, 2018

daviddias left a comment

dignifiedquire Apr 10, 2018

dignifiedquire Apr 10, 2018

dignifiedquire Apr 10, 2018

Mr0grog Apr 16, 2018

dignifiedquire Apr 10, 2018

jacobheun Apr 10, 2018

jacobheun commented Apr 16, 2018

dignifiedquire left a comment

Mr0grog left a comment

Mr0grog Apr 17, 2018

Mr0grog Apr 17, 2018

Mr0grog Apr 17, 2018

Mr0grog Apr 17, 2018

Mr0grog Apr 17, 2018

jacobheun commented Apr 17, 2018

daviddias left a comment


		#### `lock.open (dir, callback)`

		Sets the lock if one does not already exist.

feat(lock): allow for custom lock options #162

feat(lock): allow for custom lock options #162

Conversation

jacobheun commented Apr 9, 2018 • edited Loading

daviddias commented Apr 9, 2018

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Apr 16, 2018

dignifiedquire left a comment

Choose a reason for hiding this comment

Mr0grog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Apr 17, 2018

daviddias left a comment

Choose a reason for hiding this comment

jacobheun commented Apr 9, 2018 •

edited

Loading