Make the PinnerConnector great again #1086

chmanie · 2019-04-21T06:32:34Z

Description

This PR fixes a few things related to the pinner. Wow this took quite some time but I got it!

New stuff ✨

It works again!
pinion and its dependencies (ipfsd, wss proxy) are now started on yarn dev

Changes 🏗

Updated orbit-db to commit 86b12cdd048ea7d5fa5a615adf6a2126d3084794 (shorter store names)
Updated pinion to commit f98c1bbf5103aa79b63a3370c1824a5d329f908b (adjusted scripts and updated dependencies)
Fixed a few issues in PinnerConnector and raceAgainstTimeout
Made the Store <-> PinnerConnector connection more resilient

Deletions ⚰️

I removed a test that just tested itself.

Notes 📒

Please wipe your node_modules folder and update the submodules after this is merged:

rm -rf node_modules
yarn --pure-lockfile
git submodule update --recursive

To the reviewers: Please test that it works for you!

Resolves #1077

JamesLefrere

This looks good! The only thing I think is worth a look is the debug env var for pinner; it'd be better if that were configurable.

JamesLefrere · 2019-04-21T09:20:43Z

src/utils/async.js

@@ -17,7 +17,10 @@ export const raceAgainstTimeout = async (
    timeout = setTimeout(() => reject(throwError), ms);
  });
  try {
-    return Promise.race([timeoutPromise, promise]);
+    // To be able to use the async error handling here, we need to explicitly


Wow, new to me, great find (and thanks for the comment).

Yeah idk, might have to do with Babel transpilation? We might want to test this

Had a quick google, I was suspecting it could have been, but haven't found anything exactly like this.

JamesLefrere · 2019-04-21T09:36:23Z

scripts/start_all.js

+    stdio: 'pipe',
+    env: {
+      ...process.env,
+      DEBUG: 'pinner:*',


Maybe we could get this from process.env under another name? Perhaps we won't want debugging on by default.

Yeah you might be right. There's a lot of output there.

JamesLefrere · 2019-04-21T09:38:28Z

src/lib/ipfs/PinnerConnector.js

@@ -156,6 +168,17 @@ class PinnerConnector extends EventEmitter {
    this._roomMonitor.on('error', log);
  }

+  async ready() {


Looks good 👍

JamesLefrere · 2019-04-21T09:38:53Z

src/lib/ipfs/PinnerConnector.js

@@ -179,12 +202,15 @@ class PinnerConnector extends EventEmitter {
      type: PIN_ACTIONS.LOAD_STORE,
      payload: { address },
    });
-    return raceAgainstTimeout(
+    const [heads] = await raceAgainstTimeout(


Huh, yeah, I don't know how it worked before either 🤷‍♂️

chmanie · 2019-04-21T09:57:08Z

Did you try this locally @JamesLefrere?

JamesLefrere · 2019-04-21T10:02:38Z

To the reviewers: Please test that it works for you!

Oops, I didn't see this. Will do 👍

chmanie · 2019-04-22T08:14:06Z

I'm putting this back into in-progress as it seems we're far from done here.

chmanie · 2019-04-23T04:45:21Z

On hold until @thiagodelgado111 merges the orbit-db upstream PR

todo · 2019-05-05T03:47:27Z

it might be worthwhile finding out why we need to debounce this in the first place

https://github.com/JoinColony/colonyDapp/blob/aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3/src/lib/database/stores/Store.js#L46-L51

This comment was generated by todo based on a `@todo` comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

todo · 2019-05-05T03:47:30Z

Improve error modes for failed pinned store requests

This could be dangerous in case of an unfinished replication. We have to account for that Quick fix could be to just also wait for the full replication, which might be a performance hit

https://github.com/JoinColony/colonyDapp/blob/aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3/src/lib/database/stores/Store.js#L70-L75

This comment was generated by todo based on a `@todo` comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

todo · 2019-05-24T16:14:15Z

Improve replication check

This is not super accurate as the pinner could have the same number of heads but different ones. This can be improved but checking other internal states (like orbit stores do?). For now it's probably ok. Also, maybe it doesn't really need to be super accurate, as we're replicating anyways.

https://github.com/JoinColony/colonyDapp/blob/0b278dacfec0083273c43b9e4a77e3faf40d9d1b/src/lib/database/stores/Store.js#L111-L116

This comment was generated by todo based on a `@todo` comment in 0b278dacfec0083273c43b9e4a77e3faf40d9d1b in #1086. cc @JoinColony.

thiagodelgado111 · 2019-06-07T17:41:44Z

One thing that I noticed though:

14:32:08 › /Users/thiagodelgado/code/colonyDapp/node_modules/tree-kill/index.js:83
    ps.stdout.on('data', function (data) {
              ^
TypeError: Cannot read property 'on' of undefined
    at buildProcessTree (/Users/thiagodelgado/code/colonyDapp/node_modules/tree-kill/index.js:83:15)
    at /Users/thiagodelgado/code/colonyDapp/node_modules/tree-kill/index.js:104:11
    at Array.forEach (<anonymous>)
    at ChildProcess.onClose (/Users/thiagodelgado/code/colonyDapp/node_modules/tree-kill/index.js:99:31)
    at ChildProcess.emit (events.js:189:13)
    at maybeClose (internal/child_process.js:970:16)
    at Socket.stream.socket.on (internal/child_process.js:389:11)
    at Socket.emit (events.js:189:13)
    at Pipe._handle.close (net.js:597:12)

This seems to be a problem with node-tree-kill (pkrumins/node-tree-kill#17) and shouldn't block us from merging the PR. Just putting it out there. In fact, I think I've mentioned it in the past and someone told me it was a known issue

chmanie · 2019-06-10T16:03:16Z

Hmm, @thiagodelgado111 I think that's our fault. Let me see.

Until - orbitdb/orbitdb#606 and - orbitdb-archive/orbit-db-store#38 are merged

* Instead of relying on our Orbit patch to listen for `peer.exchange` events (which can be unreliable), wait for at least one head to appear locally with a timeout (horrible, but slightly less unreliable).

chmanie added the needs-code-review label Apr 21, 2019

chmanie requested review from thiagodelgado111 and JamesLefrere April 21, 2019 06:32

chmanie self-assigned this Apr 21, 2019

JamesLefrere approved these changes Apr 21, 2019

View reviewed changes

JamesLefrere added under-review and removed needs-code-review labels Apr 21, 2019

chmanie added in-progress and removed under-review labels Apr 22, 2019

chmanie force-pushed the bug/1077-pinion-connection branch from 6bdbf32 to 0d4f9ce Compare April 23, 2019 04:44

chmanie added on-hold and removed in-progress labels Apr 23, 2019

chmanie force-pushed the bug/1077-pinion-connection branch from 0d4f9ce to f47fac9 Compare April 27, 2019 09:34

chmanie added in-progress and removed on-hold labels Apr 30, 2019

chmanie force-pushed the bug/1077-pinion-connection branch from f47fac9 to 73808ef Compare April 30, 2019 05:14

JamesLefrere mentioned this pull request May 3, 2019

Request to work on Task Wiring #516

Closed

1 task

chmanie force-pushed the bug/1077-pinion-connection branch 2 times, most recently from fae39c8 to e325a60 Compare May 20, 2019 18:01

chmanie requested a review from JamesLefrere May 24, 2019 16:16

chmanie force-pushed the bug/1077-pinion-connection branch from 0b278da to 2a13fb1 Compare May 24, 2019 16:18

thiagodelgado111 added the under-review label May 24, 2019

chmanie force-pushed the bug/1077-pinion-connection branch from 0e9bd11 to 624c520 Compare May 24, 2019 21:53

Christian Maniewski and others added 23 commits June 10, 2019 18:20

Update dependencies and pinion submodule

0dcff1f

Start ipfsd, pinion, wssproxy in yarn dev

edfd131

Fix PinnerConnector, make store loading more resilient

ee16d6e

Remove avatarCache (not used)

c2819a5

Patch orbit-db and orbit-db-store

7dd83ea

Until - orbitdb/orbitdb#606 and - orbitdb-archive/orbit-db-store#38 are merged

Upgrade ava to 1.4.1

d248259

Rework pinning mechanism

55864db

Debounce pinning

fbc58f2

Update pinion to TS version

0911899

Fix and improve pinner store replication

465a409

Remove orbit patches

20119e9

Tweak heads timeout

4f530a3

Add replication hack

6da1abd

* Instead of relying on our Orbit patch to listen for `peer.exchange` events (which can be unreliable), wait for at least one head to appear locally with a timeout (horrible, but slightly less unreliable).

Update pinion

f7cd5c5

Do not use destructuring for process.env

68ba415

Will this fix the E2E tests?

4b8292d

Improve behaviour when pinner is offline

c3567ae

Make arbitrary processes skippable in start_all

e2d1a24

Use plain jsipfs instead of docker ipfsd for e2e

b590563

Remove parallelism

4cc1454

Thiago said I can remove this

323c750

Set pinion to hash dcd1f96cdb3900d4be4c2ce664fc3fd25c62cb0a

090f6fc

Fix start_all: kill ipfsd process on error

5d51c7b

chmanie force-pushed the bug/1077-pinion-connection branch from 1107a91 to 5d51c7b Compare June 10, 2019 16:21

chmanie merged commit 1fc84c3 into master Jun 10, 2019

todo bot mentioned this pull request Jun 10, 2019

Improve replication check #1322

Closed

chmanie deleted the bug/1077-pinion-connection branch June 10, 2019 16:39

chmanie removed the under-review label Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the PinnerConnector great again #1086

Make the PinnerConnector great again #1086

chmanie commented Apr 21, 2019 •

edited

JamesLefrere left a comment

JamesLefrere Apr 21, 2019

chmanie Apr 21, 2019

JamesLefrere Apr 21, 2019

JamesLefrere Apr 21, 2019

chmanie Apr 22, 2019

JamesLefrere Apr 21, 2019

JamesLefrere Apr 21, 2019

chmanie commented Apr 21, 2019

JamesLefrere commented Apr 21, 2019

chmanie commented Apr 22, 2019

chmanie commented Apr 23, 2019

todo bot commented May 5, 2019

todo bot commented May 5, 2019

todo bot commented May 24, 2019

thiagodelgado111 commented Jun 7, 2019

chmanie commented Jun 10, 2019

Make the PinnerConnector great again #1086

Make the PinnerConnector great again #1086

Conversation

chmanie commented Apr 21, 2019 • edited

Description

JamesLefrere left a comment

Choose a reason for hiding this comment

JamesLefrere Apr 21, 2019

Choose a reason for hiding this comment

chmanie Apr 21, 2019

Choose a reason for hiding this comment

JamesLefrere Apr 21, 2019

Choose a reason for hiding this comment

JamesLefrere Apr 21, 2019

Choose a reason for hiding this comment

chmanie Apr 22, 2019

Choose a reason for hiding this comment

JamesLefrere Apr 21, 2019

Choose a reason for hiding this comment

JamesLefrere Apr 21, 2019

Choose a reason for hiding this comment

chmanie commented Apr 21, 2019

JamesLefrere commented Apr 21, 2019

chmanie commented Apr 22, 2019

chmanie commented Apr 23, 2019

todo bot commented May 5, 2019

it might be worthwhile finding out why we need to debounce this in the first place

This comment was generated by todo based on a @todo comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

todo bot commented May 5, 2019

Improve error modes for failed pinned store requests

This comment was generated by todo based on a @todo comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

todo bot commented May 24, 2019

Improve replication check

This comment was generated by todo based on a @todo comment in 0b278dacfec0083273c43b9e4a77e3faf40d9d1b in #1086. cc @JoinColony.

thiagodelgado111 commented Jun 7, 2019

chmanie commented Jun 10, 2019

chmanie commented Apr 21, 2019 •

edited

This comment was generated by todo based on a `@todo` comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

This comment was generated by todo based on a `@todo` comment in aa89dc396e1fbf0b29396c8d6d095ebf6fcd47a3 in #1086. cc @JoinColony.

This comment was generated by todo based on a `@todo` comment in 0b278dacfec0083273c43b9e4a77e3faf40d9d1b in #1086. cc @JoinColony.