Fix write connection reconnection issues #2850
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
big if true |
|
huge if factual |
|
enormous if accurate |
|
sizeable if veracious |
Chatterino currently does not handle disconnects of the write connection, which
causes messages to remain buffered until you manually reconnect, after which it
will send all messages at once.
This patch addresses the lack of reconnection specifically, and attempts to
refactor some of the reconnection logic to make it more robust. Backoff behavior
is introduced to *both* read/write cons to avoid reconnection loops.
To replicate the issue, simply lose your (write) connection to TMI. This can be
"simulated" using something like:
tcpkill -i eth0 host 44.226.36.141 or host 34.217.198.238 or host 100.20.159.232
An alternative/secondary approach would be to detect failures of the underlying
`sendRaw` method (as it returns false if the message can't be sent to the
connection), but better monitoring of the connection state should be sufficient.
The way the code is setup currently doesn't really make it easy to implement
this "cleanly". E.g. calls `initializeConnection` are skipped on reconnect, but
since we're reconnecting with the same parameters, this should have little
actual consequence.
|
Account switching (including updating your own OAuth token) doesn't work properly, because the disconnect triggered by Working on a fix. |
This avoids reconnect behavior when e.g. switching / updating accounts.
Felanbird
reviewed
Jun 2, 2021
Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com>
|
The behavior should be OK now. (Also shouldn't have force pushed a formatting mistake, oops.) |
pajlada
reviewed
Jun 6, 2021
pajlada
reviewed
Jun 6, 2021
pajlada
requested changes
Jun 6, 2021
Avoids an already running pingTimer being called early in the connection and ensures we're always in a sensible state when opening a connection. Improve comments as requested.
|
Shuffled some state resets around and improved the comments. Let me know if that's sufficient. |
Perfect, thank you. I'll merge this into master and have people take this for a spin in the nightly version! |
pajlada
approved these changes
Jun 6, 2021
zneix
added a commit
to SevenTV/chatterino7
that referenced
this pull request
Jun 6, 2021
Now we're on commit 2f568b8; Changes from upstream we pulled: - Minor: Searching for users in the viewer list now searches anywhere in the user's name. (Chatterino#2861) - Minor: Now shows deletions of messages like timeouts (Chatterino#1155, Chatterino#2841) - Minor: Added a link to accounts page in settings to "You need to be logged in to send messages" message. (Chatterino#2862) - Minor: Switch to Twitch v2 emote API for animated emote support. (Chatterino#2863) - Bugfix: Fix reconnecting when IRC write connection is lost (Chatterino#1831, Chatterino#2356, Chatterino#2850) - Bugfix: Fixed bit emotes not loading in some rare cases. (Chatterino#2856)
zneix
added a commit
to SevenTV/chatterino7
that referenced
this pull request
Jun 25, 2021
Now we're on commit 0021290; Changes from upstream we pulled: - Bugfix: Fix reconnecting when IRC write connection is lost (Chatterino#1831, Chatterino#2356, Chatterino#2850, Chatterino#2892) - Bugfix: Fixed subscription emotes showing up incorrectly in the emote menu. (Chatterino#2905) Changes added in Chatterino7 only: - Major: Added 7tv badges. (2154981) - Minor: Fixed potential issues preventing 7tv emotes from loading (c0b1117)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ready for review/feedback. There's a lot of room for (additional) refactoring on the connection logic, but I figured I'd at least fix the issue first.
Pull request checklist:
CHANGELOG.mdwas updated, if applicableDescription
Chatterino currently does not handle disconnects of the write connection, which
causes messages to remain buffered until you manually reconnect, after which it
will send all messages at once.
This patch addresses the lack of reconnection specifically, and attempts to
refactor some of the reconnection logic to make it more robust. Backoff behavior
is introduced to both read/write cons to avoid reconnection loops.
To replicate the issue, simply lose your (write) connection to TMI. This can be
"simulated" using something like:
An alternative/secondary approach would be to detect failures of the underlying
sendRawmethod (as it returns false if the message can't be sent to theconnection), but better monitoring of the connection state should be sufficient.
The way the code is setup currently doesn't really make it easy to implement
this "cleanly". E.g. calls
initializeConnectionare skipped on reconnect, butsince we're reconnecting with the same parameters, this should have little
actual consequence.