openbci-archive / OpenBCI_NodeJS

Node.js SDK for the all OpenBCI Biosensor Boards
https://www.npmjs.com/package/openbci
137 stars 52 forks source link

Graceful termination #142

Open alexcastillo opened 7 years ago

alexcastillo commented 7 years ago

Hey AJ,

I'm working on making openbci reactive and noticed how the board is fully terminated on the example files:

https://github.com/OpenBCI/OpenBCI_NodeJS/blob/master/examples/debug/debug.js https://github.com/OpenBCI/OpenBCI_NodeJS/blob/master/examples/getStreamingDaisy/getStreamingDaisy.js

Are there any use cases where the way to fully terminate is different? Why not include these artifacts in the library itself? Seems like a lot of boilerplate.

Best,

Alex

andrewjaykeller commented 7 years ago

Are you looking for how to not terminate connecting on disconnect? Just don't call disconnect? Bottom line: what ever you want to change, submit a proposal and I'll merge it so long as the test pass!

alexcastillo commented 7 years ago

It should always terminate the process on disconnect, no? I can work on a PR.

Basically, this part taken from the example should be included in the library:

function exitHandler (options, err) {
  if (options.cleanup) {
    if (verbose) console.log('clean');
    ourBoard.removeAllListeners();
    /** Do additional clean up here */
  }
  if (err) console.log(err.stack);
  if (options.exit) {
    if (verbose) console.log('exit');
    ourBoard.disconnect().catch(console.log);
  }
}

if (process.platform === 'win32') {
  const rl = require('readline').createInterface({
    input: process.stdin,
    output: process.stdout
  });

  rl.on('SIGINT', function () {
    process.emit('SIGINT');
  });
}

// do something when app is closing
process.on('exit', exitHandler.bind(null, {
  cleanup: true
}));

// catches ctrl+c event
process.on('SIGINT', exitHandler.bind(null, {
  exit: true
}));

// catches uncaught exceptions
process.on('uncaughtException', exitHandler.bind(null, {
  exit: true
}));
andrewjaykeller commented 7 years ago

Ahhhh ok I see what your saying, hmm yea would love to see a PR for this

andrewjaykeller commented 7 years ago

is this still needed? should we make a different disconnect function? like disconnectGraceful?

alexcastillo commented 7 years ago

I think the current disconnect should handle all of this. Can you think of one scenario where the user won't need to disconnect gracefully?

andrewjaykeller commented 7 years ago

well the example you copied just removes the listeners. I more implement that in the examples to give people a place to clean up code. so you want a call to remove all listeners in disconnect? Do you want the process.on()'s implemented? I'm struggling to figure out what I need to add to close this issue for you.

baffo32 commented 7 years ago

He's saying if the process.on stuff is needed, it should go into the library and be automatic, calling disconnect. User can then handle cleanup using disconnect event and using the library this way is simpler and quicker.

andrewjaykeller commented 7 years ago

ah the only thing that is really needed is removing any attached event emitters. can you do that from the module?

baffo32 commented 7 years ago

shouldn't it call disconnect so the dongle stops streaming? you can do anything in a module you can do in a main file

andrewjaykeller commented 7 years ago

I guess the question is what is it... Are you saying the module shouod be able to detect these three different ways to kill a program? These are used for like making sure if you hit control C, the board disconnects.

andrewjaykeller commented 7 years ago

Like I make an electron application with this so I would not want to use the process.on because I have different hooks that get fired on app quit.