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

Heartbeat and reconnect opcodes #115

Merged
merged 3 commits into from
Nov 28, 2017
Merged

Heartbeat and reconnect opcodes #115

merged 3 commits into from
Nov 28, 2017

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Nov 26, 2017

Occasionally the gateway will request a heartbeat, and the client must respond otherwise you'll be logged out and get OPCODE_INVALID_SESSION, requiring a reconnect to get everything working again (in this state you appear offline and receive no messages, but are still able to send outgoing messages). It's pretty common and happens to me multiple times per day when DMing with friends. Possibly related to #95?

This PR triggers a reconnect when requested, and also handles the heartbeat request so it doesn't happen in the first place!

@sm00th
Copy link
Owner

sm00th commented Nov 26, 2017

Thanks, looks good. I think this might also solve #71. I think OPCODE_RECONNECT should use imcb_log, not imcb_error as it is hardly an error. Originally I wanted to make reconnect on this opcode invisible to user, but at this point this solution is just as good.

How tested is it on your side? I think I'd run it for a couple of days before merging.

@arcnmx
Copy link
Contributor Author

arcnmx commented Nov 26, 2017

I've only been running it for 24 hours at this point - I got fed up yesterday morning that I couldn't message anyone for 5 to 20 minutes without getting silently disconnected. Noticed warnings in the log about unhandled heartbeat, closely followed by unhandled invalid session. After applying the fix I have not run into that issue again so far.

I still get Error: Failed to read data occasionally so that seems like an unrelated problem.

@arcnmx
Copy link
Contributor Author

arcnmx commented Nov 28, 2017

This is likely also related to #75

@sm00th sm00th merged commit da7ac86 into sm00th:master Nov 28, 2017
@sm00th
Copy link
Owner

sm00th commented Nov 28, 2017

Didn't notice anything bad while running this and the code looks good. Thanks again, great work. Now we'll need to see if it helps fix #75 and #95.

@arcnmx arcnmx deleted the heartbeat branch December 9, 2017 16:50
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.

2 participants