Refactor js-repo away from callbacks leaning on async/await #189

zcstarr · 2019-02-11T01:38:38Z

Initial pull request for the async await refactor. Some notes.

I don't anticipate test to past just yet. There are upstream dependencies on async-multihashing and the various datastore interfaces. I modeled the interface changes based off of the current inflight async api refactor PRs for the dependencies(multiformats/js-multihashing-async#37, ipfs/interface-datastore#25, ipfs/js-datastore-level#12).

Another thing to note is that some of the error handling for opening/deleting a repo, might be up for question. It was a bit unclear what errors should be handled, given the ongoing refactor.

js-ipfs-repo/src/index.js

Line 71 in 41e042b

(cb) => this.root.open(ignoringAlreadyOpened(cb)),

Let me know what you think and what needs to be patched up or changed, happy to jam on it. If the interface seems good. I'll try to push testing the interface as far as I can go prior to the upstream package changes landing.

jacobheun

Overall I think the updates look good so far

jacobheun · 2019-02-12T12:46:22Z

src/index.js

+    return this.root.open()
+      .then(() => this.config.set(buildConfig(config)))
+      .then(() => this.spec.set(buildDatastoreSpec(config)))
+      .then(() => this.version.set(repoVersion))


This stack could probably just become await calls to make it cleaner.

alanshaw

Apologies, I haven't finished reviewing this but there's enough here that I thought I'd submit it and re-review when I have another moment.

alanshaw · 2019-02-14T13:23:10Z

src/blockstore.js

-        store.put(k, block.data, callback)
+      return store.has(k).then((exists) => {
+        if (exists) { return }
+        return store.put(k, block.data)
      })


Please await instead:

const exists = await store.has(k) if (exists) return return store.put(k, block.data)

alanshaw · 2019-02-14T13:44:10Z

src/blockstore.js

-        })
-
-        batch.commit(callback)
+      const batch = await store.batch()


We don't need to await a batch object - this should be sync.

alanshaw · 2019-02-14T14:24:35Z

src/blockstore.js

-
-        batch.commit(callback)
+      const batch = await store.batch()
+      const newKeys = await Promise.all(keys.filter((k) => { store.has(k.key) }))


Suggested change

const newKeys = await Promise.all(keys.filter((k) => { store.has(k.key) }))

const newKeys = (await Promise.all(keys.map(async k => {

const exists = await store.has(k.key)

return exists ? null : k

}))).filter(Boolean)

alanshaw · 2019-02-14T14:27:14Z

src/blockstore.js

+          const otherCid = cidToOtherVersion(cid)
+          if (!otherCid) return false
+          return store.has(cidToDsKey(otherCid))
+        })


Please use await

alanshaw · 2019-02-14T14:32:49Z

src/blockstore.js

+     * @param {Block} block
+     * @returns {Promise<void>}
+     */
+    put (block) {


Please use async keyword since this is an async API. If the caller is using traditional then/catch and the function throws then it'll cause an uncaught exception. e.g.

const b = 'INVALID BLOCK' repo.put(b).catch(errHandler) // uncaught exception, errHandler is not called

alanshaw · 2019-02-14T14:34:41Z

src/blockstore.js

     */
-    has (cid, callback) {
+    has (cid) {


Please use async keyword since this is an async API. See previous comment for explanation.

alanshaw · 2019-02-14T14:34:50Z

src/blockstore.js

     */
-    delete (cid, callback) {
+    delete (cid) {


Please use async keyword since this is an async API. See previous comment for explanation.

alanshaw · 2019-02-26T08:56:20Z

src/index.js

   */
-  _isInitialized (callback) {
+  async _isInitialized () {


should now return true/false

AuHau · 2019-03-07T00:06:04Z

src/index.js

+    await this.root.open()
+    await this.config.set(buildConfig(config))
+    await this.spec.set(buildDatastoreSpec(config))
+    await this.version.set(repoVersion)
  }

  /**
   * Open the repo. If the repo is already open no action will be taken.
   * If the repo is not initialized it will return an error.


I believe that "If the repo is already open no action will be taken." is not really correct, because according to the L77, it will throw an error. Maybe lets update it?

Good catch, the comment is definitely off 👍

Maybe the error thrown should be bit more custom (eq. creating a new category in ./errors/index.js) so people could detect if the opening of repo failed because it is locked and handle it? But not sure if it is in the scope of this PR though...

dirkmc · 2019-03-13T20:46:57Z

src/version.js

-        callback(null, parseInt(buf.toString().trim(), 10))
-      })
+    async get () {
+      const buf = await this.get(versionKey)


This line causes a stack overflow, I think it should be

Suggested change

const buf = await this.get(versionKey)

const buf = await store.get(versionKey)

It does, I have some pending changes stored locally for this, that I haven't pushed back up yet. Do you mind if I drop a PTAL when I push the various fixes I have going locally, this PR was a bit tricker to write and is probably more error prone in places, because I wrote it before the other PRs. Thanks for looking into it!

No worries, I just noticed this when testing some code that has this as a dependency. The rest of it looks great to me.

AuHau · 2019-03-13T21:53:46Z

src/index.js

-        cb()
-      }
-    ], (err) => callback(err))
+    await this.apiAddr.delete()


This refactored part will fail if the /api does not exists; see original this.apiAddr.delete(ignoringNotFound(cb)).

dirkmc · 2019-03-14T14:57:14Z

src/index.js

-      (cb) => this.spec.set(buildDatastoreSpec(config), cb),
-      (cb) => this.version.set(repoVersion, cb)
-    ], callback)
+    await this.root.open()


Just a reminder to make sure we cover the case where the repo is already open

dirkmc · 2019-03-14T15:09:37Z

src/index.js

+      this.keys = backends.create('keys', path.join(this.path, 'keys'), this.options)
+      this.closed = false
+      log('all opened')
+    } catch (err) {
      if (err && this.lockfile) {


I think you still want to throw here if there is no lock file.

dirkmc · 2019-03-14T15:10:23Z

src/index.js

+        } catch (err2) {
+          log('error removing lock', err2)
+        }
+        this.lockfile = null


I think you only want to set this.lockfile to null if there was not an error thrown from this._closeLock()

dirkmc · 2019-03-14T15:31:31Z

src/config.js

      }

      if (value === undefined || Buffer.isBuffer(value)) {
-        return callback(new Error('Invalid value type'))
+        throw new Error('Invalid value type')
      }

      setQueue.push({


I'd suggest using a p-queue so that you can return a Promise here

AuHau · 2019-03-20T17:42:50Z

package.json

@@ -49,15 +49,14 @@
    "rimraf": "^2.6.2"
  },
  "dependencies": {
-    "async": "^2.6.1",
    "base32.js": "~0.1.0",
    "bignumber.js": "^8.0.2",
    "cids": "~0.5.7",
    "datastore-core": "~0.6.0",
    "datastore-fs": "~0.7.0",


Could you please add a dependency on ipfs/js-datastore-fs#22 ?

AuHau · 2019-04-28T16:37:51Z

package.json

-    "dlv": "^1.1.2",
-    "interface-datastore": "~0.6.0",
+    "err-code": "^1.1.2",
+    "interface-datastore": "git://github.com/ipfs/interface-datastore.git#refactor/async-iterators",


As the refactor/async-iterators PR was merged and the branch deleted this is outdated and breaks things.

achingbrain · 2019-06-04T18:25:39Z

Superseded by #199

alanshaw self-requested a review February 11, 2019 17:47

alanshaw mentioned this pull request Feb 11, 2019

Awesome Endeavour: Async Iterators ipfs/js-ipfs#1670

Closed

jacobheun reviewed Feb 12, 2019

View reviewed changes

alanshaw requested changes Feb 26, 2019

View reviewed changes

zcstarr force-pushed the master branch from 9bd1ff0 to bd99272 Compare February 26, 2019 19:23

AuHau reviewed Mar 7, 2019

View reviewed changes

dirkmc reviewed Mar 13, 2019

View reviewed changes

AuHau reviewed Mar 13, 2019

View reviewed changes

dirkmc reviewed Mar 14, 2019

View reviewed changes

dirkmc mentioned this pull request Mar 14, 2019

chore: callbacks -> async / await ipfs/js-ipfs-block-service#85

Merged

2 tasks

AuHau reviewed Mar 20, 2019

View reviewed changes

AuHau mentioned this pull request Mar 29, 2019

fix: memlock handling existing locks #195

Closed

Zane Starr and others added 8 commits April 1, 2019 09:30

Refactor js-repo away from callbacks leaning on async/await

71f1aac

Explicitly label async and fix putMany

0e5a899

Fix await on init

1c97679

Refactor repo initialization to return boolean

3ae87fe

Label async functions

90c9c74

Add async await to browser test

16ce060

fix: several bug fixes

85c9214

fix: add async label to async functions

442b2cf

zcstarr force-pushed the master branch from ee574fb to 442b2cf Compare April 1, 2019 16:33

AuHau reviewed Apr 28, 2019

View reviewed changes

achingbrain mentioned this pull request May 30, 2019

Refactor/async await #199

Merged

achingbrain closed this Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor js-repo away from callbacks leaning on async/await #189

Refactor js-repo away from callbacks leaning on async/await #189

zcstarr commented Feb 11, 2019

jacobheun left a comment

jacobheun Feb 12, 2019

alanshaw left a comment

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 14, 2019

alanshaw Feb 26, 2019

AuHau Mar 7, 2019

zcstarr Mar 7, 2019

AuHau Mar 7, 2019

dirkmc Mar 13, 2019

zcstarr Mar 13, 2019

dirkmc Mar 13, 2019

AuHau Mar 13, 2019

dirkmc Mar 14, 2019 •

edited

Loading

dirkmc Mar 14, 2019

dirkmc Mar 14, 2019

dirkmc Mar 14, 2019

AuHau Mar 20, 2019

AuHau Apr 28, 2019

achingbrain commented Jun 4, 2019

-      const newKeys = await Promise.all(keys.filter((k) => { store.has(k.key) }))
+      const newKeys = (await Promise.all(keys.map(async k => {
+        const exists = await store.has(k.key)
+        return exists ? null : k
+      }))).filter(Boolean)

	const buf = await this.get(versionKey)
	const buf = await store.get(versionKey)

Refactor js-repo away from callbacks leaning on async/await #189

Refactor js-repo away from callbacks leaning on async/await #189

Conversation

zcstarr commented Feb 11, 2019

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc Mar 14, 2019 • edited Loading

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

achingbrain commented Jun 4, 2019

dirkmc Mar 14, 2019 •

edited

Loading