From afb216c202feb143ce70c74c16fa50ca93da6157 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 16 Mar 2023 11:13:30 -0400 Subject: [PATCH] Remove no-op send_command for Redis replication. (#15274) With Redis commands do not need to be re-issued by the main process (they fan-out to all processes at once) and thus it is no longer necessary to worry about them reflecting recursively forever. --- changelog.d/15272.misc | 2 +- changelog.d/15274.misc | 1 + synapse/replication/tcp/handler.py | 26 +------- .../replication/tcp/test_remote_server_up.py | 63 ------------------- 4 files changed, 3 insertions(+), 89 deletions(-) create mode 100644 changelog.d/15274.misc delete mode 100644 tests/replication/tcp/test_remote_server_up.py diff --git a/changelog.d/15272.misc b/changelog.d/15272.misc index 7a3ef323e..f7c0276ec 100644 --- a/changelog.d/15272.misc +++ b/changelog.d/15272.misc @@ -1 +1 @@ -Remove unused class `DirectTcpReplicationClientFactory`. +Clean-up direct TCP replication code. diff --git a/changelog.d/15274.misc b/changelog.d/15274.misc new file mode 100644 index 000000000..f7c0276ec --- /dev/null +++ b/changelog.d/15274.misc @@ -0,0 +1 @@ +Clean-up direct TCP replication code. diff --git a/synapse/replication/tcp/handler.py b/synapse/replication/tcp/handler.py index d03a53d76..2290b3e6f 100644 --- a/synapse/replication/tcp/handler.py +++ b/synapse/replication/tcp/handler.py @@ -625,23 +625,6 @@ class ReplicationCommandHandler: self._notifier.notify_remote_server_up(cmd.data) - # We relay to all other connections to ensure every instance gets the - # notification. - # - # When configured to use redis we'll always only have one connection and - # so this is a no-op (all instances will have already received the same - # REMOTE_SERVER_UP command). - # - # For direct TCP connections this will relay to all other connections - # connected to us. When on master this will correctly fan out to all - # other direct TCP clients and on workers there'll only be the one - # connection to master. - # - # (The logic here should also be sound if we have a mix of Redis and - # direct TCP connections so long as there is only one traffic route - # between two instances, but that is not currently supported). - self.send_command(cmd, ignore_conn=conn) - def new_connection(self, connection: IReplicationConnection) -> None: """Called when we have a new connection.""" self._connections.append(connection) @@ -689,21 +672,14 @@ class ReplicationCommandHandler: """ return bool(self._connections) - def send_command( - self, cmd: Command, ignore_conn: Optional[IReplicationConnection] = None - ) -> None: + def send_command(self, cmd: Command) -> None: """Send a command to all connected connections. Args: cmd - ignore_conn: If set don't send command to the given connection. - Used when relaying commands from one connection to all others. """ if self._connections: for connection in self._connections: - if connection == ignore_conn: - continue - try: connection.send_command(cmd) except Exception: diff --git a/tests/replication/tcp/test_remote_server_up.py b/tests/replication/tcp/test_remote_server_up.py deleted file mode 100644 index b75fc05fd..000000000 --- a/tests/replication/tcp/test_remote_server_up.py +++ /dev/null @@ -1,63 +0,0 @@ -# Copyright 2020 The Matrix.org Foundation C.I.C. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -from typing import Tuple - -from twisted.internet.address import IPv4Address -from twisted.internet.interfaces import IProtocol -from twisted.test.proto_helpers import MemoryReactor, StringTransport - -from synapse.replication.tcp.resource import ReplicationStreamProtocolFactory -from synapse.server import HomeServer -from synapse.util import Clock - -from tests.unittest import HomeserverTestCase - - -class RemoteServerUpTestCase(HomeserverTestCase): - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - self.factory = ReplicationStreamProtocolFactory(hs) - - def _make_client(self) -> Tuple[IProtocol, StringTransport]: - """Create a new direct TCP replication connection""" - - proto = self.factory.buildProtocol(IPv4Address("TCP", "127.0.0.1", 0)) - transport = StringTransport() - proto.makeConnection(transport) - - # We can safely ignore the commands received during connection. - self.pump() - transport.clear() - - return proto, transport - - def test_relay(self) -> None: - """Test that Synapse will relay REMOTE_SERVER_UP commands to all - other connections, but not the one that sent it. - """ - - proto1, transport1 = self._make_client() - - # We shouldn't receive an echo. - proto1.dataReceived(b"REMOTE_SERVER_UP example.com\n") - self.pump() - self.assertEqual(transport1.value(), b"") - - # But we should see an echo if we connect another client - proto2, transport2 = self._make_client() - proto1.dataReceived(b"REMOTE_SERVER_UP example.com\n") - - self.pump() - self.assertEqual(transport1.value(), b"") - self.assertEqual(transport2.value(), b"REMOTE_SERVER_UP example.com\n")