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

dependency-initialiser calls process.exit() #243

Closed
mvberg opened this issue Jun 29, 2016 · 5 comments
Closed

dependency-initialiser calls process.exit() #243

mvberg opened this issue Jun 29, 2016 · 5 comments
Assignees

Comments

@mvberg
Copy link

mvberg commented Jun 29, 2016

If a plugin fails to initialize on server.start(), for example the Redis cache connector cannot connect to it's host - the entire process is exited - this seems pretty extreme - thoughts?

@WolframHempel
Copy link
Member

In my mind that makes sense :-). By configuring a redis connector, you say that you want your data to be persisted. If deepstream can't connect to it, it knows that it wont be able to store your data and everything will be lost if the server is restarted.
At that point, it doesn't make much sense to continue. Let's rather shut down and fix the problem.

@ronag
Copy link

ronag commented Jun 30, 2016

Related to #207

@timaschew
Copy link
Contributor

@mvberg
I totally agree with you that to exit the whole process is not a good idea, at least for the usage via an API.

In the new release of deepstream (1.0.0) we provide a CLI. To exit the process in that case makes sense for me. But for the API we will consider to not exit the process, instead to just throw an exception.
So the user can catch it and for instance fix the issue and try to server.start() again.

@mvberg
Copy link
Author

mvberg commented Jun 30, 2016

Thank you guys for the feedback.

@timaschew Yep, this is exactly my use case, deepstream isn't the only part of my setup on the server side.

@WolframHempel

At that point, it doesn't make much sense to continue. Let's rather shut down and fix the problem.

If you comment out the exit calls, deepstream actually recovers very nicely, firing all ready events (tested with redis-message and redis-cache) and starts itself up when redis comes back online :)

@yasserf
Copy link
Contributor

yasserf commented Jun 30, 2016

Problem is though that if just commenting out the lines would result in deepstream potentially just hanging, hoping to connect at some point in the future.

Ideally you would be using a cache/db/msg cluster with more redundant entrypoints and not a single point of failure, and hence deepstream would be able to connect to one of them within the time allocated.

You could also disable the functionality but increasing the dependency timeout to something huge, which would avoid exiting. It doesn't solve the error issue, but if an error occurs during a connection is means something is probably wrong enough to deserve a process stop/restart.

The part that I totally agree with is that exiting a process for a node developer must be confusing since you can't actually deal with anything in your own code. It might be worth us bubbling an error up to our cli and exiting there, and allow deepstream users to catch the error and deal accordingly as well

@timaschew timaschew self-assigned this Jul 5, 2016
@timaschew timaschew mentioned this issue Jul 5, 2016
yasserf pushed a commit that referenced this issue Jul 5, 2016
* emit a fatal event instead of exit the process
also try to close the dependency which might started to do some work, like the rulecache

* minor fixes:
- print help if unkown command was used
- fix printing option name for invalid integers
- fix passing lib-dir option in detach mode
- attempt to fix detached mode on linux binaries
- remove color option from root level
- delete init-script

* Revert "emit a fatal event instead of exit the process"

This reverts commit 53dffe7.

* throw error (uncaughtException) instead of exit the process
@yasserf yasserf closed this as completed Jul 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants