Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Loosen up init requirements: accept already initialised repos #808

Merged
merged 4 commits into from
Mar 23, 2017

Conversation

victorb
Copy link
Member

@victorb victorb commented Mar 23, 2017

This PR will make it so if you start a node twice with the same path, it won't crash when trying to initialize a new repo. Synonym with doing ipfs daemon --init with go-ipfs in terminal.

@victorb victorb added the status/in-progress In progress label Mar 23, 2017
@victorb
Copy link
Member Author

victorb commented Mar 23, 2017

Currently failing with:

  1) create node can start node twice without crash:
     Error: Not able to start from state: uninitalized
      at IPFS.start (src/core/components/start.js:23:19)
      at tasks.push (src/core/boot.js:106:33)
      at node_modules/async/internal/parallel.js:27:9
      at replenish (node_modules/async/internal/eachOfLimit.js:64:17)
      at iterateeCallback (node_modules/async/internal/eachOfLimit.js:49:17)
      at node_modules/async/internal/onlyOnce.js:12:16
      at node_modules/async/internal/parallel.js:32:13
      at apply (node_modules/lodash/_apply.js:15:25)
      at node_modules/lodash/_overRest.js:32:12
      at node_modules/async/internal/once.js:12:16
      at nextTask (node_modules/async/waterfall.js:15:29)
      at node_modules/async/waterfall.js:22:13
      at apply (node_modules/lodash/_apply.js:15:25)
      at node_modules/lodash/_overRest.js:32:12

src/core/boot.js Outdated
tasks.push((cb) => self.init(initOptions, cb))
tasks.push((cb) => {
console.log('Doing init')
self._repo.exists((err, exists) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't make much sense as it's the same thing as in the else branch

src/core/boot.js Outdated
if (err) cb(err)
console.log('exists', err, exists)
if (exists) {
cb(null, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to call maybeOpenRepo if you don't do init

src/core/boot.js Outdated
@@ -49,7 +49,16 @@ module.exports = (self) => {

if (doInit) {
self.log('boot:doInit')
tasks.push((cb) => self.init(initOptions, cb))
tasks.push((cb) => {
self._repo.exists((err, exists) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are still doing the same call twice in the if (doInit). you can just call repo.exists and do the if doInit check later down

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this

self._repo.exists((err, hasRepo) => {
  if (err) {
    return done(err)
  }
  if (doInit && !hasRepo) {
    tasks.push((cb) => self.init(initOptions, cb))
  }
  if (hasRepo) {
    tasks.push(maybeOpenRepo)
  } 
  if (setConfig) {
    // ...
})

src/core/boot.js Outdated
tasks.push((cb) => self.init(initOptions, cb))
tasks.push((cb) => {
self._repo.exists((err, exists) => {
if (err) cb(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to return and please on with braces on multiple lines

@victorb
Copy link
Member Author

victorb commented Mar 23, 2017

Ready to be checked and eventually merged now :)

@dignifiedquire
Copy link
Member

@victorbjelkholm did you see my comments?

@victorb victorb force-pushed the fix/init-twice branch 3 times, most recently from 43f8c3f to 4f763ef Compare March 23, 2017 16:02
src/core/boot.js Outdated
if (repoExists) {
tasks.push(maybeOpenRepo)
}
next(null, repoExists)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the next function anymore :)

src/core/boot.js Outdated
}
if (repoExists) {
tasks.push(maybeOpenRepo)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still a bug, next needs to be called with hasRepo not true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dignifiedquire I've tried that and then I get bunch of failures

WARNING, trying to set config on uninitialized repo, maybe forgot to set "init: true"
WARNING, trying to start ipfs node on uninitialized repo, maybe forgot to set "init: true"

src/core/boot.js Outdated
if (doInit && !repoExists) {
tasks.push((cb) => self.init(initOptions, cb))
}
if (repoExists) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check for !doInit as well

@daviddias daviddias changed the title Prevent crash when starting node twice Loosen up init requirements: accept already initialised repos Mar 23, 2017
@daviddias daviddias mentioned this pull request Mar 23, 2017
22 tasks
series(tasks, done)
}
})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to the whole commit :)

@daviddias daviddias merged commit 835e993 into master Mar 23, 2017
@daviddias daviddias deleted the fix/init-twice branch March 23, 2017 23:56
@daviddias daviddias removed the status/in-progress In progress label Mar 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants