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

Validate config and provide good error messages if something is wrong #1193

Closed
thiagodelgado111 opened this issue Jan 27, 2018 · 5 comments · Fixed by #1239
Closed

Validate config and provide good error messages if something is wrong #1193

thiagodelgado111 opened this issue Jan 27, 2018 · 5 comments · Fixed by #1239
Assignees
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up

Comments

@thiagodelgado111
Copy link

  • Version: 0.27.7
  • Platform:
    ProductName: Mac OS X
    ProductVersion: 10.12.6
    BuildVersion: 16G1114

Type: Bug

Severity: Medium

Description:

If an invalid protocol is specified in Ipfs' config constructor argument, it will be persisted and then pre-start keeps failing due to the invalid protocol – even after changing the config.

Steps to reproduce the error:

› node src/index.js

Swarm listening on /ip4/127.0.0.1/tcp/4001/ipfs/YYYYYYYYYY
I am ready!
Waiting for peers...
peers:  [ '/ip4/127.0.0.1/tcp/9999/ws/ipfs/XXXXXXXXXX' ]

Protocol used on Addresses.Swarm config list /ip6/::/etcp/4001

› node src/index.js

/Users/thiagodelgado/code/ipfs-playground/node_modules/multiaddr/src/protocols-table.js:17
    throw new Error('no protocol with name: ' + proto)
    ^

Error: no protocol with name: etcp

After fixing the config object

› node src/index.js

/Users/thiagodelgado/code/ipfs-playground/node_modules/multiaddr/src/protocols-table.js:17
    throw new Error('no protocol with name: ' + proto)
    ^

Error: no protocol with name: etcp

Same error even though not using that protocol anymore. Editing .jsipfs/config file removing the bad line solved it.

› node src/index.js

Swarm listening on /ip4/127.0.0.1/tcp/4001/ipfs/YYYYYYYYYY
I am ready!
Waiting for peers...
peers:  [ '/ip4/127.0.0.1/tcp/9999/ws/ipfs/XXXXXXXXXX' ]
@dryajov
Copy link
Member

dryajov commented Jan 27, 2018

@thiagodelgado111 I'm not sure if I understand - removing the non existent protocol etcp fixed it, or did you run into some other issue? I believe that the behavior is expected (tho perhaps no very user friendly?) - multiaddr will throw if it can't recognise a protocol.

@thiagodelgado111
Copy link
Author

thiagodelgado111 commented Jan 27, 2018

removing the non existent protocol etcp fixed it, or did you run into some other issue?

Hi @dryajov, thanks for helping! So, removing the non-existent protocol from the config file fixed it!

Nevertheless, It took me a while to figure out the problem was in the config file though, would it be possible to either validate the config object before writing it, ignore the version read from disk if it fails and use the config object or ask users to check the config file when it happens?

@dryajov
Copy link
Member

dryajov commented Jan 27, 2018

Thanks for clarifying @thiagodelgado111 - I agree ipfs should behave in a more user-friendly way when handling issues like this. Perhaps this is a good place to start a discussion around this? //cc @hacdias @daviddias @richardshneider @victorbjelkholm

@daviddias
Copy link
Member

No need for discussion, @thiagodelgado111 is right. The API should empower the developer by providing good error messages and validate inputs. That's how we get good UX.

@daviddias daviddias changed the title pre-start fails if an invalid protocol is used once in configuration Validate config and provide good error messages if something is wrong Feb 19, 2018
@daviddias daviddias added exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked labels Feb 19, 2018
@alanshaw
Copy link
Member

A config error cannot be caught, or listened for and will cause the process to exit.

const IPFS = require('ipfs')

console.log('IPFS node starting')
let node

try {
  node = new IPFS({
    "config": {
      "Addresses": {
        "Swarm": [ "rekt" ]
      }
    }
  })
  console.log('invalid config did not throw')
} catch (err) {
  console.log('invalid config threw error')
  throw err
}

node.once('ready', () => {
  console.log('IPFS node ready...')
})

node.on('error', (err) => {
  console.log('invalid config emitted error', err)
})

process.on('uncaughtException', (err) => {
  console.log('invalid config caused uncaughtException')
  throw err
})

Output:

$ npm ls ipfs && node ipfs-config-bug.js 
/Users/alan/Desktop/test
└── [email protected] 

IPFS node starting
invalid config did not throw
invalid config caused uncaughtException
/Users/alan/Desktop/test/ipfs-config-bug.js:30
  throw err
  ^

Error: no protocol with name: rekt
    at Protocols (/Users/alan/Desktop/test/node_modules/multiaddr/src/protocols-table.js:17:11)
    at stringToStringTuples (/Users/alan/Desktop/test/node_modules/multiaddr/src/codec.js:45:19)
    at stringToBuffer (/Users/alan/Desktop/test/node_modules/multiaddr/src/codec.js:170:13)
    at Object.fromString (/Users/alan/Desktop/test/node_modules/multiaddr/src/codec.js:178:10)
    at new Multiaddr (/Users/alan/Desktop/test/node_modules/multiaddr/src/index.js:39:25)
    at Multiaddr (/Users/alan/Desktop/test/node_modules/multiaddr/src/index.js:27:12)
    at config.Addresses.Swarm.forEach (/Users/alan/Desktop/test/node_modules/ipfs/src/core/components/pre-start.js:27:22)
    at Array.forEach (<anonymous>)
    at waterfall (/Users/alan/Desktop/test/node_modules/ipfs/src/core/components/pre-start.js:26:34)
    at nextTask (/Users/alan/Desktop/test/node_modules/async/waterfall.js:16:14)

@alanshaw alanshaw self-assigned this Feb 28, 2018
@ghost ghost added status/in-progress In progress and removed status/ready Ready to be worked labels Feb 28, 2018
@ghost ghost removed the status/in-progress In progress label Mar 12, 2018
MicrowaveDev pushed a commit to galtproject/js-ipfs that referenced this issue May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants