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

[MM-62925] introduce websocket client-side ping #8633

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

davidkrauser
Copy link

@davidkrauser davidkrauser commented Feb 25, 2025

This PR is a continuation of #8619. The PR was migrated to a branch on the upstream repository because E2E tests can't execute on branches from forked repositories.

Summary

This PR introduces new functionality on the client side to send PING messages over the websocket. If the server doesn't respond within PING_INTERVAL (currently 30 seconds), the connection is closed and re-created. This will allow us to find broken connections more quickly.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62925

This is related to a similar change in the webapp client: mattermost/mattermost#30293

Testing at the Application Layer

This change can be manually verified using a mitmproxy that intermittently drops websocket messages. This script will drop every other PONG message from the server, starting with the first:

import time
from mitmproxy import ctx

class WebSocketDropper:
    def __init__(self):
        self.drop_pong = True

    def websocket_message(self, flow):
        if not "pong" in flow.websocket.messages[-1].content.decode('utf-8'):
            return
        
        if self.drop_pong:
            ctx.log.info("Dropping server PONG response")
            flow.websocket.messages[-1].drop()
            self.drop_pong = False
        else:
            self.drop_pong = True

addons = [WebSocketDropper()]

This proxy can be run with:

mitmproxy --set verbosity=3 -s PATH_TO_MITMPROXY_PYTHON_SCRIPT --listen-port 8080 --mode reverse:http://localhost:8065

That will start delivering requests on localhost:8080 to a local mattermost instance on the default port.

Additionally to get around some CORS checks, the local instance needs to have its base URL set to http://LOCAL_HOST_IP:8080 in the system console. The mobile app should connect to http://LOCAL_HOST_IP:8080.

With everything up and running, you'll see:

  • After 30 seconds, the client sends a PING, but the server PONG message is dropped
  • After another 30 seconds, the websocket will be closed and re-opened (since the PONG was never received).
  • After another 30 seconds, the client will successfully complete a PING/PONG interaction with the server
  • Repeat

Testing at the Network Layer

We can also verify that the connection is re-established quickly after a failed attempt to send a PING message. This can happen if the packets are being rejected.

To test on a Mac:

  1. Set up a standard development instance of mattermost running on localhost at port 8065
  2. Open the webapp in a web browser (at localhost:8065)
  3. Configure the admin user and set the base url to http://LOCAL_HOST_IP:8065 in the system console.
  4. Login to the mobile app at http://LOCAL_HOST_IP:8065.
  5. In a terminal, execute sudo pfctl -e && echo "block return proto tcp to port 8065" | sudo pfctl -f -. This will block all tcp traffic to port 8065.
  6. Watch the mobile app report that it has lost it's connection with the server.
  7. In a terminal, execute sudo pfctl -d. This will start allowing tcp traffic again
  8. Verify a the mobile app connection is re-established.

Checklist

  • Added or updated unit tests (required for all new features)
  • Have run E2E tests by adding label E2E iOS tests for PR.

Release Note

Added a client-side ping to the websocket to detect broken connections more quickly.

@davidkrauser davidkrauser added 2: Dev Review Requires review by a core commiter E2E iOS tests for PR Run iOS E2E Detox tests labels Feb 25, 2025
@davidkrauser
Copy link
Author

@enahum and @larkox this is a new PR with the same contents as #8619 that you already reviewed. The old PR was using a branch from a forked repo, which doesn't work with the E2E testing infrastructure.

Would you mind lending your approvals on this PR, too? 🙂

@github-actions github-actions bot removed the E2E iOS tests for PR Run iOS E2E Detox tests label Feb 25, 2025
@yasserfaraazkhan yasserfaraazkhan added the E2E Android tests for PR Run Android E2E Detox tests label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter E2E Android tests for PR Run Android E2E Detox tests release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants