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

If Slack client closes, exit the hubot process #215

Merged
merged 3 commits into from
Nov 12, 2015

Conversation

eriklindebratt
Copy link
Contributor

Since Slack client seems to have troubles reconnecting occasionally (e.g. #203), this seems like the only way to solve it right now. I'm running Hubot using supervisor and in the case of Slack client dropping its connection it seems to me that having the Hubot process exit and get restarted from e.g. supervisor is the way to go.

Since Slack client seems to have troubles reconnecting occasionally (e.g. slackapi#203), this seems like the only way to solve it right now. I'm running Hubot using supervisor and in the case of Slack client dropping its connection it seems to me that having the Hubot process exit and get restarted from supervisor is the way to go.
@farski
Copy link

farski commented Nov 6, 2015

Would this be better as an optional behavior change?

@knpwrs
Copy link

knpwrs commented Nov 7, 2015

I'd like to see this merged but I agree with @farski that it should be behind an option.

@eriklindebratt
Copy link
Contributor Author

@farski @KenPowers Yep, I agree. Would you prefer something like an env var (e.g. HUBOT_SLACK_EXIT_ON_DISCONNECT=true bin/hubot --adapter slack)?

@knpwrs
Copy link

knpwrs commented Nov 7, 2015

Either that or if hubot could support subargs then we could have:

bin/hubot --adapter slack [ --exit-on-disconnect ]

But I think environment variables are more likely.

@christopherRiddersater
Copy link

Sorry for jumping in like this.
Behind an option would be nice and as @KenPowers points out, a subarg would be nice but either would be a great improvement.

Set environment variable to e.g. `HUBOT_SLACK_EXIT_ON_DISCONNECT=true`
to have the whole Hubot process exit if the Slack client disconnects.
@eriklindebratt
Copy link
Contributor Author

@farski @KenPowers @christopherRiddersater I added a check for an environment variable HUBOT_SLACK_EXIT_ON_DISCONNECT and that is used as a flag in slack.coffee on whether the Hubot process should exit when the Slack client disconnects. Thoughts?

@farski
Copy link

farski commented Nov 9, 2015

@eriklindebratt Looks reasonable to me. Only thing is exit_process_on_disconnect should probably be exitProcessOnDisconnect by convention.

@eriklindebratt
Copy link
Contributor Author

@farski Ah missed that one, thanks.

@christopherRiddersater
Copy link

Looks good to me.

@spoon99
Copy link

spoon99 commented Nov 12, 2015

+1

@bradam12
Copy link

+1 This would help my monitoring for sure. My hubot tends to not connect back, so it sits there not doing anything.

@evansolomon
Copy link
Contributor

Is this issue pretty much the opposite of #127?

@bradam12
Copy link

Seems so, but is configurable on what behavior you want. If you want it to try reconnecting, leave it as is. If you want it to exit so supervisor/script/equivalent can detect it died and spin Hubot back up, go for it and turn the option on.

@knpwrs
Copy link

knpwrs commented Nov 12, 2015

The behavior I was observing is that my hubot would never reconnect.

@bradam12
Copy link

Same. Mine never tries reconnecting (seemingly), so I'd rather let it exit and respin it back up.

@ivarrian
Copy link

Same as @KenPowers . The process is up but the last log I see is INFO Slack client closed, waiting for reconnect , but it doesn't ever reconnect. I have setup foreverjs to restart the process when it dies, but it actually sticks around doing nothing.

@knpwrs
Copy link

knpwrs commented Nov 12, 2015

That said, the preferable solution for the long term would be to figure out why hubot isn't reconnecting.

@bradam12
Copy link

Agreed.

@evansolomon
Copy link
Contributor

Seems reasonable to me

@christopherRiddersater
Copy link

Yes, I agree about the long term solution and I think that the short time solution may prove useful for other scenarios in the future as well.

@mjsz
Copy link
Contributor

mjsz commented Nov 12, 2015

This looks reasonable as a stopgap, thanks all. I will plan to merge and do a patch release with this included later today.

mjsz added a commit that referenced this pull request Nov 12, 2015
If Slack client closes, exit the hubot process
@mjsz mjsz merged commit 596c418 into slackapi:master Nov 12, 2015
@eriklindebratt
Copy link
Contributor Author

Sweet @mjsz, thanks!

@christopherRiddersater
Copy link

@mjsz Great! Many thanks!

@MrJoy
Copy link

MrJoy commented Jul 6, 2016

Any notion of when a new release that includes this will be pushed to NPM?

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

Successfully merging this pull request may close these issues.

10 participants