noflo / noflo-nodejs

Command-line tool for running NoFlo programs on Node.js
91 stars 36 forks source link

insufficient error handling for missing fields in flowhub.json #14

Closed ensonic closed 9 years ago

ensonic commented 9 years ago

Lets assue you missed the "user" field in your flowhub.json. When starting the runtime it prints

 npm start

> noflo-local-rt@0.0.1 start /home/ensonic/projects/noflo/noflo-local-rt
> node node_modules/.bin/noflo-nodejs

NoFlo runtime listening at ws://192.168.0.47:3569
Using /home/ensonic/projects/noflo/noflo-local-rt for component loading

So the errors thrown in https://github.com/the-grid/flowhub-registry/blob/master/index.js#L28-39 are lost. I see two options: 1.) we don't touch flowhub-registry and only add a try/catch around https://github.com/noflo/noflo-nodejs/blob/master/bin/noflo-nodejs#L55 . Down side is that we have two places we handle the error.

2.) we change flowhub-registry to not throw, but invoke the callback:

 if (!this.runtime.user) {
-  throw new Error('Runtime registration requires a user UUID')
+  callback (new Error('Runtime registration requires a user UUID'));
   return;
 }

Finally I wonder if https://github.com/noflo/noflo-nodejs/blob/master/bin/noflo-nodejs#L58 should call process.exit(1) instead of return? How useful is the runtime when its cannot announce itself?

jonnor commented 9 years ago

Last point first: Generally I think Flowhub registry registration must to be optional. Even when registration is explicitly enabled, I think the runtime should continue b Because the primary purpose of the runtime is the code/service it runs (not being able to be debugged/re-programmed from Flowhub). Registration also just needs to happen once (for a given IP setting).

jonnor commented 9 years ago

For fixing the user error handling, I think 2) is the way to go, when there is a callback, one should always call it with Error instead of throwing

ensonic commented 9 years ago

Send a pr: https://github.com/the-grid/flowhub-registry/pull/3