Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful termination #142

Open
alexcastillo opened this issue Mar 19, 2017 · 11 comments
Open

Graceful termination #142

alexcastillo opened this issue Mar 19, 2017 · 11 comments

Comments

@alexcastillo
Copy link

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
Copy link

andrewjaykeller commented Mar 19, 2017

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
Copy link
Author

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
Copy link

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

@andrewjaykeller
Copy link

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

@alexcastillo
Copy link
Author

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
Copy link

andrewjaykeller commented Mar 24, 2017

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
Copy link
Contributor

baffo32 commented Mar 24, 2017

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
Copy link

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

@baffo32
Copy link
Contributor

baffo32 commented Mar 24, 2017

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
Copy link

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
Copy link

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.

andrewjaykeller pushed a commit to andrewjaykeller/OpenBCI_NodeJS that referenced this issue Mar 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants