search

phaser-discord / administration

Random admin stuff
0 stars 2 forks source link

Update Index.js to emit Error Events #27

Closed TheRealRyGuy closed 6 years ago

TheRealRyGuy commented 6 years ago

Should fix Issue #23 , good luck with your bot!

jdotrjs commented 6 years ago

eep. I wanted to test this before merging it but need to actually roll it out and wait until the bot dies again ... which I forgot to do.

I'm a bit worried that this will log the error but not re-initiate the connection to the discord server; I think we may need to drop a new discordClient.login(cfg.discord.token) call in the error handling function but will have to poke at the API to verify. About to head to bed tonight but will update this ticket as I progress on getting this version rolled out for testing.

@RyGuy88228, thanks so much for making this PR. If you have an answer to my questions that'd be super neat since it sounds like you've already looked into this kind of issue with bots :)

jdotrjs commented 6 years ago

New version deployed; now we just need to

  1. wait until it dies
  2. observe behavior
  3. merge or make changes as a result
jdotrjs commented 6 years ago

(still hasn't crashed... i guess now that there is maybe a fix in queued our bug is a bit fearful to show it's head)

TheRealRyGuy commented 6 years ago

@jdotrjs Look into Client#destroy and Client#login, however catching these errors should mean that your bot should not go down, so it's connection will not need to be re-initialized

jdotrjs commented 6 years ago

Per comments in the bug we don't crash as a result of errors which is great. This means that the oncall/offcall management stuff still works but the issue marking does not. I assume that this is because we don't reestablish the websocket connection. (edit: actually I'm not sure why this would be the case given that the message and reaction signals come over the same channel so it's clearly not the connection thing now)

I'm going to merge this as it's a significant improvement over the behavior on master but will leave the underlying issue open.

@RyGuy88228, Thanks for the PR! It was annoying to have to reset the server every time this happened so I'm glad that we're half way to fixed now :grinning: