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

fix: websocket not receiving messages after some time #791

Merged

Conversation

vitorhugods
Copy link
Member


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

After some time, the websocket dies. It completely stops receiving messages without any sings of errors, exceptions, or anything else being called.

Causes

As we do not send messages to the backend, it seems that the backend expects at least some pings every so often to keep the connection alive.
After some time without receiving anything, the backend just leaves the connection in a limbo and doesn't send any more messages through it.

Solutions

Add a ping every so many seconds.
It seems that @makingthematrix also went through this problem back in 2019 for the Scala app (see this commit), and 30s has been the magic number that carried the Scala app until now.

Unfortunately OkHttp doesn't handle dynamic pinging, so it must be configured directly into the HttpEngine, and it seems that the configuration in WebSockets is just ignored if you're using OkHttp.

So, even though adding it to OkHttp fixes it for Android and JVM, I've added it to Ktor as well so it also works for other platforms.

Note: During my investigation I also added a lower-level logging to the WebSocket and I guess there's no harm keeping it.

Testing

Manually tested as we can't yet mock WebSockets using Ktor.


PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 13, 2022

Unit Test Results

   175 files   -   17     175 suites   - 17   24s ⏱️ +3s
   788 tests  - 134     780 ✔️  - 135  8 💤 +1  0 ±0 
1 478 runs  +556  1 469 ✔️ +554  9 💤 +2  0 ±0 

Results for commit 0401355. ± Comparison against base commit 212198a.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #791 (0401355) into develop (212198a) will decrease coverage by 0.14%.
The diff coverage is 6.06%.

@@              Coverage Diff              @@
##             develop     #791      +/-   ##
=============================================
- Coverage      58.44%   58.29%   -0.15%     
  Complexity       666      666              
=============================================
  Files            431      432       +1     
  Lines          11138    11170      +32     
  Branches        1122     1122              
=============================================
+ Hits            6510     6512       +2     
- Misses          4150     4180      +30     
  Partials         478      478              

@vitorhugods
Copy link
Member Author

Labeler is currently broken: srvaroa/labeler#43.

Seems to be caused by: niemeyer/gopkg#63

Copy link
Member

@ohassine ohassine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo! Nice catch 👌

Copy link
Member

@MohamadJaara MohamadJaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@vitorhugods vitorhugods merged commit 6ab1048 into develop Aug 15, 2022
@vitorhugods vitorhugods deleted the fix/websocket_not_receiving_messages_after_some_time branch August 15, 2022 13:33
MohamadJaara added a commit that referenced this pull request Aug 16, 2022
* fix: websocket not receiving messages after some time

* chore: reduce ping interval to 20s

Co-authored-by: Mohamad Jaara <mohamad.jaara@wire.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants