WIP callbacks -> async / await #82

dirkmc · 2019-02-21T14:19:57Z

BREAKING CHANGE: This change replaces callbacks with async / await

I refactored a few things while I was making this change:

queue.js
- functionally equivalent, but now uses Iterators instead of an async/queue
- stops any running workers if an error is thrown
- has a mechanism to stop a query once the required number of results have been fetched
- ensure peers are unique by peer id (before they were only unique by Object reference)
providers.js
- now ensures that addProvider(), getProviders() and _cleanup() are synchronized (using a p-queue)
- now cleans up any expired entries (previously expired entries would only be cleaned up if all peer ids for a given CID had expired)
- add items to the cache any time we go to the datastore (previously we would cache on addProvider() but not on getProviders())
random-walk.js
- simplify scheduling mechanism
- fix a bug where we check a non-existent variable on start
getMany()
- Fix bug where requesting a single value would fail if the value was not found in the local datastore (ie before it wouldn't go out to the DHT to look for it. Now it does)
stop()
- Ensure all running queries are stopped when the DHT is stopped
Moved all pull-streams into a single class so that we can easily replace with Iterators when the Connection interface has been updated

vasco-santos

Thanks for this PR @dirkmc . So great to have this progress 💪 Also thanks for improving some docs not related =D

I made a first pass through the initial src files, as this is a really big PR, and left a few comments

In a next step we could get rid of pull-streams. Maybe the first step could be to convert the pull-streams that are from other modules to streams (and separate all them to a separate file, for easy plug and play afterward). When this module need to expose a pull-stream to other module, we may implement a node stream as well, and have a converter in the end that we will be able to remove afterward as well. What do you think?

package.json

src/index.js

dirkmc · 2019-03-05T22:44:05Z

Yes I think that makes sense 👍

vasco-santos

I reviewed src/private and src/random-walk now and left a few more comments

src/private.js

BREAKING CHANGE: All places in the API that used callbacks are now replaced with async/await

kumavis · 2019-04-22T11:45:39Z

I don't mean to disparage the great effort here but in my experience large on-going refactors are a bad idea when working with multiple contributors. You end up in merge conflict hell and in resolving them its easy to run over patches that were merged after the refactor started.

Also seems this one has also outgrown its original async/await focus and has become a something more, possibly now a sort of too-big-to-fail PR that blocks or at least complicates all development.

I don't mean to sound dire, I've just lived through too many of these. And I'm mostly an outsider here so I'm not trying to tell someone what to do but I'd recommend (1) resolving and merging this asap (2) using the code here to break this down into multiple smaller PRs

happy to do my part and not just comment : )

I anticipate working very closely with the dht this month

kumavis · 2019-04-22T12:18:21Z

src/query.js

+ }
+
+ const run = new Run(this.dht, this.key, this.makePathQuery, this._log)
+ this.run = run


seems like this.run accidentally clashes with the run function defined on the Query class. I guess this could be an intentional override, but if so it should be heavily commented because its surprising

@dirkmc could you clarify if this is intentional or not

@kumavis it's not intentional - thanks for pointing it out.

I think it's best to hold off on updating this PR until the dependencies are released

opened an issue #114

kumavis · 2019-04-22T12:19:54Z

src/query.js

+ * unnecessarily.
+ */
+ stop () {
+ this.run && this.run.stop()


this seems like a mistake (as I elsewhere noted about this.run). I would expect this.run to always be truthy because run is defined on the class

kumavis · 2019-04-22T12:54:01Z

package.json

 "base32.js": "~0.1.0",
 "chai-checkmark": "^1.0.1",
 "cids": "~0.5.7",
 "debug": "^4.1.1",
 "err-code": "^1.1.2",
 "hashlru": "^2.3.0",
 "heap": "~0.2.6",
- "interface-datastore": "~0.6.0",
+ "interface-datastore": "ipfs/interface-datastore#refactor/async-iterators",


issue with interface-datastore dep ref

➜ npm i npm ERR! code 1 npm ERR! Command failed: git checkout refactor/async-iterators npm ERR! error: pathspec 'refactor/async-iterators' did not match any file(s) known to git

looks like merged and branch deleted 👍 ipfs/interface-datastore#25

not published yet though, I'll point to the commit for now

kumavis · 2019-04-22T13:06:28Z

package.json

- "multihashing-async": "~0.5.2",
- "peer-id": "~0.12.2",
- "peer-info": "~0.15.1",
+ "multihashing-async": "multiformats/js-multihashing-async#feat/async-iterators",


published as 0.7.0 👍

kumavis · 2019-04-22T13:07:24Z

Theres some git branch dep hell going on as async/await branches gets merged deleted

kumavis · 2019-04-22T13:30:02Z

endeavour: ipfs/js-ipfs#1670
blocking PRs in deps

vasco-santos · 2019-04-22T14:56:57Z

Thanks for your feedback @kumavis

We will need to update this PR with the new changes, and we will do it while the remaining dependencies are not ready. Hopefully, we will be able to get this PR more ready to merge in the next month

dirkmc · 2019-04-29T12:16:23Z

@kumavis if you are interested in helping out with DHT work it would be very welcome. This issue is one we will need to implement in both Go and Javascript: libp2p/go-libp2p-kad-dht#323

kumavis · 2019-04-30T02:40:31Z

@dirkmc hey/ took a stab at adding your refactors in a non-breaking way to reduce conflicts #108

kumavis · 2019-05-23T01:36:55Z

@dirkmc theres a lot of great work in here that i dont want to get lost, but extracting the high level changes from the diff is a bit difficult -- if there are any additional fixes or changes in this PR that are not enumerated in the PR description, could you add them so I can make sure they aren't lost? Thanks!

kumavis · 2019-06-03T09:55:51Z

heres where we're at getting these changes in master (may have missed something)

src/query/workerQueue
- cb -> async
- uses Iterators instead of an async/queue
- stops any running workers if an error is thrown
- has a mechanism to stop a query once the required number of results have been fetched
- ensure peers are unique by peer id (before they were only unique by Object reference)
providers
- cb -> async
- now ensures that addProvider(), getProviders() and _cleanup() are synchronized (using a p-queue)
- now cleans up any expired entries (previously expired entries would only be cleaned up if all peer ids for a given CID had expired)
- add items to the cache any time we go to the datastore (previously we would cache on addProvider() but not on getProviders())
random-walk
- cb -> async
- simplify scheduling mechanism
- fix a bug where we check a non-existent variable on start
dht/index
- cb -> async
- getMany()
  - Fix bug where requesting a single value would fail if the value was not found in the local datastore (ie before it wouldn't go out to the DHT to look for it. Now it does)
  - async Iterator interface
- stop()
  - Ensure all running queries are stopped when the DHT is stopped
Move all pull-streams into a single class so that we can easily replace with Iterators when the Connection interface has been updated
- src/rpc
- src/providers
- src/network

* refactor(async): providers refactor from #82 * test: providers dedupes redundant providers * added feedback from PR review * aegir - increase bundle size to 222kB

vasco-santos · 2019-08-25T15:15:43Z

@kumavis @dirkmc does it make sense to close this PR and work from the current state of things?

We already have a lot of work done here, as well as the current status check list from @kumavis to iterate. I hope to get the interface-connection work finished soon, so that we can avance with this

dirkmc · 2019-08-26T14:10:36Z

Makes sense to me - thanks for the help with getting this into master @kumavis

dirkmc force-pushed the feat/async-await branch 7 times, most recently from 579eda7 to 1247bc4 Compare February 27, 2019 19:29

dirkmc marked this pull request as ready for review February 27, 2019 19:32

dirkmc force-pushed the feat/async-await branch from 1247bc4 to 15e939e Compare February 27, 2019 22:23

dirkmc mentioned this pull request Mar 4, 2019

Closest peers query is not efficient #86

Open

vasco-santos suggested changes Mar 5, 2019

View reviewed changes

package.json Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

vasco-santos suggested changes Mar 6, 2019

View reviewed changes

src/private.js Outdated Show resolved Hide resolved

src/private.js Outdated Show resolved Hide resolved

dirkmc added 3 commits March 7, 2019 10:54

chore: callbacks -> async / await

a45e6fc

BREAKING CHANGE: All places in the API that used callbacks are now replaced with async/await

refactor: use iterables instead of Promises where possible

da2e000

fix: fix tests

0d8e8e0

dirkmc force-pushed the feat/async-await branch from a119f32 to 0d8e8e0 Compare March 7, 2019 16:09

dirkmc added 4 commits March 8, 2019 14:41

refactor: more efficient concurrent queueing

58e4037

refactor: move all pull-stream usage into one class

940f68d

fix: stop all queries when DHT stops

9059b61

fix: ensure Promise rejections are handled

1f048d7

dirkmc force-pushed the feat/async-await branch from 679d71c to 1f048d7 Compare March 17, 2019 23:09

dirkmc mentioned this pull request Mar 18, 2019

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

Closed

fix: allow more random walks after error

f7a24db

dirkmc mentioned this pull request Mar 28, 2019

Ensure queries stop after error or success #93

Merged

dirkmc mentioned this pull request Apr 11, 2019

Limit scope of queries to k closest peers #97

Merged

jacobheun mentioned this pull request Apr 18, 2019

fix: random walk #104

Merged

kumavis mentioned this pull request Apr 22, 2019

async/await syntax #106

Closed

kumavis reviewed Apr 22, 2019

View reviewed changes

kumavis mentioned this pull request Apr 29, 2019

async refactor - refactor internal methods as async #108

Merged

This was referenced May 18, 2019

refactor(async): _findNProviders query function #119

Merged

refactor(async): Query #120

Merged

kumavis mentioned this pull request Jun 3, 2019

Async Refactor: query functions are now async functions #132

Merged

kumavis added a commit that referenced this pull request Jun 3, 2019

refactor(async): providers refactor from #82

013fc62

kumavis added a commit that referenced this pull request Jun 3, 2019

refactor(async): providers refactor from #82

1531149

kumavis added a commit that referenced this pull request Jun 3, 2019

refactor(async): providers refactor from #82

82d1661

This was referenced Jun 3, 2019

Async Refactor: providers #133

Merged

Async Refactor: random-walk #134

Merged

vasco-santos pushed a commit that referenced this pull request Jun 10, 2019

refactor(async): providers refactor

30fd44a

* refactor(async): providers refactor from #82 * test: providers dedupes redundant providers * added feedback from PR review * aegir - increase bundle size to 222kB

vasco-santos closed this Aug 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP callbacks -> async / await #82

WIP callbacks -> async / await #82

dirkmc commented Feb 21, 2019 •

edited

Loading

vasco-santos left a comment

dirkmc commented Mar 5, 2019

vasco-santos left a comment

kumavis commented Apr 22, 2019

kumavis Apr 22, 2019

kumavis Apr 29, 2019

dirkmc Apr 29, 2019

kumavis May 10, 2019

kumavis Apr 22, 2019

kumavis Apr 22, 2019

kumavis Apr 22, 2019 •

edited

Loading

kumavis Apr 22, 2019

kumavis Apr 22, 2019

kumavis commented Apr 22, 2019

kumavis commented Apr 22, 2019 •

edited by jacobheun

Loading

vasco-santos commented Apr 22, 2019

dirkmc commented Apr 29, 2019

kumavis commented Apr 30, 2019

kumavis commented May 23, 2019

kumavis commented Jun 3, 2019 •

edited

Loading

vasco-santos commented Aug 25, 2019

dirkmc commented Aug 26, 2019

WIP callbacks -> async / await #82

WIP callbacks -> async / await #82

Conversation

dirkmc commented Feb 21, 2019 • edited Loading

vasco-santos left a comment

Choose a reason for hiding this comment

dirkmc commented Mar 5, 2019

vasco-santos left a comment

Choose a reason for hiding this comment

kumavis commented Apr 22, 2019

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

kumavis Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Apr 22, 2019

kumavis commented Apr 22, 2019 • edited by jacobheun Loading

vasco-santos commented Apr 22, 2019

dirkmc commented Apr 29, 2019

kumavis commented Apr 30, 2019

kumavis commented May 23, 2019

kumavis commented Jun 3, 2019 • edited Loading

vasco-santos commented Aug 25, 2019

dirkmc commented Aug 26, 2019

dirkmc commented Feb 21, 2019 •

edited

Loading

kumavis Apr 22, 2019 •

edited

Loading

kumavis commented Apr 22, 2019 •

edited by jacobheun

Loading

kumavis commented Jun 3, 2019 •

edited

Loading