fix: daemon should recognize previously created repos. #212

JonKrone · 2018-02-28T00:39:08Z

While working on some go/js interop tests I noticed that a daemon spawned on an already-initialized repo doesn't report itself as initialized.

It turned out to be just a typo so I fixed that and added a test.

Update

This PR has four fixes related to proc node startup:

proc nodes error when reporting their version: here
proc nodes spawn before their IPFS instance has finished booting: here
proc nodes do not report themselves as initialized: here
daemons spawned remotely do not report themselves as initialized. here, here, here, here

dryajov · 2018-02-28T00:47:10Z

test/spawn-options.spec.js

@@ -141,6 +141,11 @@ describe('Spawn options', () => {
            expect(_ipfsd).to.exist()
            expect(_ipfsd.api).to.not.exist()

+            // proc nodes do not reuse initialized repos


I think there is no reason why we can't do this in 'proc' nodes as well.

My thought was that proc nodes always set their initialized to false (here) so having this check would avoid a failure once an issue with proc nodes (line 122 above) is resolved.

JonKrone · 2018-02-28T16:59:02Z

@dryajov Travis is failing on browser tests for my changes. There may be an obvious answer for this, but: how are browser tests being run? When I run test:browser, no tests run and the output is just an error (below).

0 passing (1m)

Error: Some tests are failing

As I understand, aegir looks for a test/browser.js file, though this project does not have one.
There are clearly browser tests being run on CI so I'm a little confused. I've checked travis config but no reference to anything but npm run test

JonKrone · 2018-02-28T17:17:52Z

Ah! Figured it out. aegir fallsback on all tests with .spec.js. Turns out my problem was not having Chrome installed! I had a development version of Chrome but it didn't recognize that.

JonKrone · 2018-02-28T18:11:47Z

@dryajov I've looked into why the browser tests are failing and think it's a bug.

The daemon that the browser's DaemonClient is communicating with is initialized but the browser's DaemonClient doesn't recognize that. If you try to call init on the client's daemon, it errors with ipfs configuration file already exists

The flow is, I think:

browser creates factory, remote: true so use FactoryClient
browser spawns daemon from factory with a repoPath that is already initialized
- browser sends request to /spawn endpoint
- ipfsd-ctl spawns a daemon from FactoryDaemon
  - This spawned daemon has initialized === true
- ipfsd-ctl responds with the {id, api} of the spawned daemon
- browser creates a DaemonClient with {id, api}
  - This DaemonClient has initialized === false
DaemonClient is the ipfsd instance with an api we can send commands through.
- calling ipfsd.init fails

Should we instead carry the initialized through the /spawn api and feed it to the DaemonClient?

JonKrone · 2018-02-28T19:45:15Z

I've just pushed changes for the above problem. Happy to change or revert it if you want me to.

There's is a race condition I was running into during browser tests for ipfsd.start on an initialized repo with a go daemon. I am working on a Windows machine and it may be something that is resolved with the /shutdown work. The daemon failed to start on top of a repo because it was still locked.

JonKrone · 2018-03-01T16:32:53Z

Current status: CI inconsistent but overall Jenkins and Travis show that things are okay.

Jenkins passed Windows 9.2 but failed 8.9 with a daemon.stop race condition (shutdown related).

Travis passed Node 6 and Node 8 tests passed but coverage failed. A similar thing happened with previous CI: Travis failed during coverage (but on different tests than this time).

Ready for review

dryajov · 2018-03-06T23:34:40Z

Pity that we don't get CircleCi builds - I'm guessing its because this is a PR from a fork. The existing CI issues indeed look "normal" - windows builds fail because of lack of #205 (as you mention).

dryajov

LGTM besides lint.

dryajov · 2018-03-06T23:36:58Z

test/spawn-options.spec.js

@@ -137,11 +137,16 @@ describe('Spawn options', () => {
          }

          f.spawn(options, (err, _ipfsd) => {
+            ipfsd = _ipfsd


I believe this will fail lint - can you run npm run lint on the repo and make sure everything passes please?

I have and do not see any failures. Were you expecting it to fail because it's assigning a global? ipfsd is defined above, on line 124 so here we are just setting it.

I moved this assignment from below because if any of the below expects failed, it would inadvertently cause the next its to fail.

Usually eslint will complain if the first line in the callback is not handling the error, but perhaps its OK since it happens right after ipfsd = _ipfsd. I would still move the assignment below the expectss since if there was an error _ipfsd would be null/empty/invalid.

dryajov · 2018-03-06T23:38:10Z

test/spawn-options.spec.js

            expect(err).to.not.exist()
            expect(_ipfsd).to.exist()
            expect(_ipfsd.api).to.not.exist()

-            ipfsd = _ipfsd
+            // proc nodes don't reuse initialized repos
+            if (fOpts.type !== 'proc') {


We should add this functionality to proc nodes - there is no reason they can't do that. But I'm fine if its in another PR. Could you file an issue to add this please?

Ahh cool. I really haven't even looked at in-proc nodes but this is an opportunity. I've filed an issue and am happy to work on it later. #214

dryajov · 2018-03-06T23:39:25Z

src/ipfsd-daemon.js

@@ -52,7 +52,7 @@ class Daemon {
    this.disposable = this.opts.disposable
    this.exec = this.opts.exec || process.env.IPFS_EXEC || findIpfsExecutable(this.opts.type, rootPath)
    this.subprocess = null
-    this.initialized = fs.existsSync(path)
+    this.initialized = fs.existsSync(this.path)


codecov · 2018-03-07T19:41:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1f3f172). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #212   +/-   ##
=========================================
  Coverage          ?   87.03%           
=========================================
  Files             ?       17           
  Lines             ?      671           
  Branches          ?        0           
=========================================
  Hits              ?      584           
  Misses            ?       87           
  Partials          ?        0

Impacted Files	Coverage Δ
src/ipfsd-client.js	`97.26% <100%> (ø)`
src/endpoint/routes.js	`81.11% <100%> (ø)`
src/factory-client.js	`60.52% <100%> (ø)`
src/ipfsd-daemon.js	`91.83% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f3f172...7756924. Read the comment docs.

JonKrone · 2018-03-07T19:48:14Z

@dryajov

I have added the fix for in-proc nodes to this PR: #214. It ended up being a two-liner:

in-proc nodes wait for their IPFS to be 'ready' during spawning
initialized state now based on repoPath's existence, same impl as regular js-ipfs daemons.

I think that they are in-line with the original intent of this PR. If we'd prefer to separate them, I am happy to revert.

Edit: Whoops, didn't run browser tests. Looking into it.

dryajov · 2018-03-07T20:36:02Z

src/factory-in-proc.js

@@ -131,6 +131,7 @@ class FactoryInProc {
    const node = new Node(options)

    series([
+      (cb) => node.exec.on('ready', cb),


please use once - this event might be triggered several times.

dryajov · 2018-03-07T20:39:10Z

src/ipfsd-in-proc.js

@@ -29,11 +30,11 @@ class Node {
    this.path = this.opts.repoPath
    this.repo = createRepo(this.path)
    this.disposable = this.opts.disposable
+    this.initialized = fs.existsSync(this.path)


this will fail in the browser, since it wouldn't be a path... Maybe we can use the ipfs-repo module here to verify it's a repo - tho I've little experience with it.

Whew! Took me a minute to realize we use package.json to define aliases for some files on browsers. That threw me for a loop.

There's a good possibility from ipfs-repo: _isInitialized(callback). I'm about to push something, just running all tests. Lemme know what you think.

daviddias · 2018-03-11T23:07:29Z

src/factory-in-proc.js

+
+    node.once('ready', () => {
+      node.version((err, _version) => {
+        if (err) { callback(err) }


needs return.

Ha! Thanks, you really have an eye for these. Perhaps we could write a lint warning for this?

@diasdavid Looks like there already is an ESLint rule for this: https://eslint.org/docs/rules/callback-return

Adding a lint rule to aegir would be a big change across our projects but it might be worth it for this one as it's not about style but functionality and could catch bugs. Have you guys looked into it before?

It might be okay to set it with error severity but we would want to pre-run some repos to check. We could alternatively phase it in as a warning and do a fix chore of key repos during that period, bumping it to an error afterwards.

I say enable the lint rule in aegir as an error and see what happens.

daviddias · 2018-03-11T23:07:55Z

src/factory-in-proc.js

+      node.version((err, _version) => {
+        if (err) { callback(err) }
+        callback(null, _version)
+      })


Same comment as #205 (comment)

daviddias · 2018-03-11T23:08:24Z

src/factory-in-proc.js

@@ -131,6 +135,19 @@ class FactoryInProc {
    const node = new Node(options)

    series([
+      (cb) => node.exec.once('ready', cb),


same comment as #205 (comment)

Something isn't quite right here. the node is created but then what we listen on the ready event is exec??

@diasdavid Can you rephrase? node.exec is the IPFS instance being run by the daemon node. What's going on is that new Node() instantiates the daemon and returns immediately with a still-booting IPFS instance under exec. We could ask consumers to wait for their in-proc daemon's IPFS to be ready but that is inconsistent with the go and js factories, which deliver ready instances.

daviddias · 2018-03-11T23:08:52Z

src/factory-in-proc.js

+        try {
+          node.repo._isInitialized(() => {
+            node.initialized = true
+            cb()


Do not call callbacks within try blocks, it results on any error in the future that gets thrown (in any part of the code) to get caught by this try block

Great! That's good to know

JonKrone · 2018-03-12T19:20:20Z

@dryajov looking at the js daemon, if you pass a custom IPFS exec: js-factory.spawn({ exec: new IPFS({}) }), the daemon returned by factory.spawn would still be booting. Do we expect users to know about this and to pass a 'ready' IPFS or wait to be ready after spawning? Could be, just curious.

https://github.com/ipfs/js-ipfsd-ctl/blob/master/src/ipfsd-daemon.js#L53
and
https://github.com/ipfs/js-ipfsd-ctl/blob/master/src/factory-daemon.js#L132

dryajov · 2018-03-12T21:50:47Z

new IPFS() is not a valid parameter for exec - it expects a class/function declaration not instantiation. Its also not a valid parameter for go or js types. So the behavior is undefined in those cases.

dryajov · 2018-03-14T22:44:42Z

@JonKrone can we get master rebased here. It should resolve the concerns that @diasdavid raised.

… test suite from failing if the first 'it' does. Also trying to run the CI again because I haven't been able to replicate its failure.

… daemons.

…overage. Interested to see if the same thing happens again.

… status.

…ready before during spawning.

…ing.\nUse ipfs-repo._isInitialized to determine whether they are or not. I'm curious it throws instead of returning false.

…. It doesn't need a try/catch at all, I mistook the behavior of _isInitialized.

…rned, not invoked.

…initialized: true'

JonKrone · 2018-03-15T18:06:41Z

PR is up to date. Jenkins failed due to a macos EADDIRINUSE error (has an issue: ipfs-inactive/jenkins#97) but everything else is ✅

I've since re-run CI and hit a couple of different issues:

macos 9.2 failed to download go-ipfs_v0.4.13_darwin-amd64.tar.gz. I saw this issue once before, many weeks ago, and I'm not sure if it is a rare intermittent issue or related to something else: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfsd-ctl/detail/PR-212/16/pipeline/14
windows 9.2 seemed to hang on [5/5] Building fresh packages... of the yarn --mutex network command for about 30 minutes. I stopped it, concerned about wasting resources. I've since learned that it may have been waiting for a build elsewhere on the network to complete before running its final step but I'm not sure. Does someone know if it was hanging or just paused? https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfsd-ctl/detail/PR-212/14/pipeline

victorb · 2018-03-15T18:11:04Z

PR is up to date. Jenkins failed due to a macos EADDIRINUSE error (has an issue: ipfs-inactive/jenkins#97) but everything else is white_check_mark

Yeah, we have something weird with aegir itself or the tests it runs that doesn't properly stop resources when it's done and some processes ends up running, so the next run on the same project + worker leads to collisions...

macos 9.2 failed to download go-ipfs_v0.4.13_darwin-amd64.tar.gz. I saw this issue once before, many weeks ago, and I'm not sure if it is a rare intermittent issue or related to something else: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfsd-ctl/detail/PR-212/16/pipeline/14

There seems to be some DNS flipping happening with dist.ipfs.io currently, which is why this error is happening, so seems intermittent.

windows 9.2 seemed to hang on [5/5] Building fresh packages... of the yarn --mutex network command for about 30 minutes.

I think windows is just very slow on installing. The --mutex network flag does indeed make sure only run one instance of yarn, but currently is locally to the worker, to prevent concurrent installs (neither yarn or npm handles those very good).

30 minutes sounds like an aweful lot though, I'll look into it.

daviddias · 2018-03-16T22:11:42Z

@JonKrone please start opening PR from branches from the repo itself to make it simpler to collaborate on a WIP PR.

daviddias · 2018-03-16T22:12:58Z

Reading the above, LGTM. Great work @JonKrone :)

JonKrone self-assigned this Feb 28, 2018

ghost added the status/in-progress In progress label Feb 28, 2018

dryajov reviewed Feb 28, 2018

View reviewed changes

JonKrone requested a review from dryajov March 1, 2018 16:29

This was referenced Mar 6, 2018

feat: jsipfs pin ipfs/js-ipfs#1249

Merged

Feat/pin ipfs/interop#15

Closed

dryajov suggested changes Mar 6, 2018

View reviewed changes

dryajov reviewed Mar 7, 2018

View reviewed changes

daviddias reviewed Mar 11, 2018

View reviewed changes

daviddias requested changes Mar 11, 2018

View reviewed changes

This was referenced Mar 13, 2018

Feat/pin tracking issue ipfs/js-ipfs#1261

Closed

In proc nodes can not be spawned on top of existing repos #214

Closed

JonKrone mentioned this pull request Mar 15, 2018

feat: jsipfs add --only-hash ipfs/js-ipfs#1233

Merged

1 task

JonKrone added 5 commits March 15, 2018 12:28

fix: daemon now recognizes previously created repos.

9b68d93

fix: prevent 2nd and 3rd 'it's in the 'spawn from a initialized repo'…

c4e5189

… test suite from failing if the first 'it' does. Also trying to run the CI again because I haven't been able to replicate its failure.

feat: remote clients receive the 'initialized' status of their remote…

d7d8e02

… daemons.

trigger CI again. Node/Browser Tests passed but failed when running c…

dc5714e

…overage. Interested to see if the same thing happens again.

doc: Add a todo and issue reference for in-proc nodes reusing repos.

f570897

JonKrone added 5 commits March 15, 2018 12:28

clean: move assignment of ipfsd below the expects which validate it's…

c5c8918

… status.

fix: js-ipfs in-process nodes now wait for their IPFS instance to be …

80adef8

…ready before during spawning.

fix: in-proc node repos now set their initialized status during spawn…

12a0e50

…ing.\nUse ipfs-repo._isInitialized to determine whether they are or not. I'm curious it throws instead of returning false.

fix: replace try/catch of repo._isInitialized with a regular callback…

9d4d29b

…. It doesn't need a try/catch at all, I mistook the behavior of _isInitialized.

fix: proc-daemon.init's err cb after replacing config was simply retu…

8613d28

…rned, not invoked.

JonKrone force-pushed the fix/initialized branch from 593ad0b to 8613d28 Compare March 15, 2018 16:54

feat(test): Test that nodes spawned without an existing repo report '…

7756924

…initialized: true'

victorb mentioned this pull request Mar 15, 2018

EADDRINUSE on macos worker ipfs-inactive/jenkins#97

Closed

daviddias approved these changes Mar 16, 2018

View reviewed changes

daviddias merged commit 2e9cf0d into ipfs:master Mar 16, 2018

ghost removed the status/in-progress In progress label Mar 16, 2018

dryajov pushed a commit that referenced this pull request Mar 23, 2018

fix: daemon should recognize previously created repos. (#212)

1841db1

victorb mentioned this pull request Aug 13, 2018

EADDRINUSE on macos worker ipfs-inactive/dev-team-enablement#105

Closed

fix: daemon should recognize previously created repos. #212

fix: daemon should recognize previously created repos. #212

Conversation

JonKrone commented Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

JonKrone Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

JonKrone commented Feb 28, 2018

JonKrone commented Feb 28, 2018 • edited Loading

JonKrone commented Feb 28, 2018 • edited Loading

JonKrone commented Feb 28, 2018

JonKrone commented Mar 1, 2018 • edited Loading

dryajov commented Mar 6, 2018

dryajov 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

codecov bot commented Mar 7, 2018 • edited Loading

Codecov Report

JonKrone commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

dryajov Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonKrone Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

JonKrone Mar 13, 2018 • 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

Choose a reason for hiding this comment

JonKrone commented Mar 12, 2018

dryajov commented Mar 12, 2018

dryajov commented Mar 14, 2018

JonKrone commented Mar 15, 2018

victorb commented Mar 15, 2018

daviddias commented Mar 16, 2018

daviddias commented Mar 16, 2018

JonKrone commented Feb 28, 2018 •

edited

Loading

JonKrone Feb 28, 2018 •

edited

Loading

JonKrone commented Feb 28, 2018 •

edited

Loading

JonKrone commented Feb 28, 2018 •

edited

Loading

JonKrone commented Mar 1, 2018 •

edited

Loading

codecov bot commented Mar 7, 2018 •

edited

Loading

JonKrone commented Mar 7, 2018 •

edited

Loading

dryajov Mar 7, 2018 •

edited

Loading

JonKrone Mar 12, 2018 •

edited

Loading

JonKrone Mar 13, 2018 •

edited

Loading