From 3da334b5cf4b56d77fc9aa8117f3efe27d243b6f Mon Sep 17 00:00:00 2001 From: "Alexander \"PapaTutuWawa" Date: Mon, 3 Apr 2023 12:46:15 +0200 Subject: [PATCH] feat(core): Remove the connection lock --- packages/moxxmpp/lib/src/connection.dart | 62 +--------- .../moxxmpp/lib/src/connection_errors.dart | 6 - packages/moxxmpp/test/helpers/xmpp.dart | 5 + packages/moxxmpp/test/xmpp_test.dart | 111 ++++++++++++++++++ 4 files changed, 120 insertions(+), 64 deletions(-) diff --git a/packages/moxxmpp/lib/src/connection.dart b/packages/moxxmpp/lib/src/connection.dart index 6ce1f7b..65f80d5 100644 --- a/packages/moxxmpp/lib/src/connection.dart +++ b/packages/moxxmpp/lib/src/connection.dart @@ -114,6 +114,7 @@ class XmppConnection { /// A policy on how to reconnect final ReconnectionPolicy _reconnectionPolicy; + ReconnectionPolicy get reconnectionPolicy => _reconnectionPolicy; /// The class responsible for preventing errors on initial connection due /// to no network. @@ -173,34 +174,9 @@ class XmppConnection { /// The logger for the class final Logger _log = Logger('XmppConnection'); - /// A value indicating whether a connection attempt is currently running or not - bool _isConnectionRunning = false; - final Lock _connectionRunningLock = Lock(); - /// Flag indicating whether reconnection should be enabled after a successful connection. bool _enableReconnectOnSuccess = false; - /// Enters the critical section for accessing [XmppConnection._isConnectionRunning] - /// and does the following: - /// - if _isConnectionRunning is false, set it to true and return false. - /// - if _isConnectionRunning is true, return true. - Future _testAndSetIsConnectionRunning() async => - _connectionRunningLock.synchronized(() { - if (!_isConnectionRunning) { - _isConnectionRunning = true; - return false; - } - - return true; - }); - - /// Enters the critical section for accessing [XmppConnection._isConnectionRunning] - /// and sets it to false. - Future _resetIsConnectionRunning() async => - _connectionRunningLock.synchronized(() => _isConnectionRunning = false); - - ReconnectionPolicy get reconnectionPolicy => _reconnectionPolicy; - bool get isAuthenticated => _isAuthenticated; /// Return the registered feature negotiator that has id [id]. Returns null if @@ -364,13 +340,6 @@ class XmppConnection { /// Attempts to reconnect to the server by following an exponential backoff. Future _attemptReconnection() async { - if (await _testAndSetIsConnectionRunning()) { - _log.warning( - '_attemptReconnection is called but connection attempt is already running. Ignoring...', - ); - return; - } - _log.finest('_attemptReconnection: Setting state to notConnected'); await _setConnectionState(XmppConnectionState.notConnected); _log.finest('_attemptReconnection: Done'); @@ -391,7 +360,7 @@ class XmppConnection { /// Called when a stream ending error has occurred Future handleError(XmppError error) async { _log.severe('handleError called with ${error.toString()}'); - + // Whenever we encounter an error that would trigger a reconnection attempt while // the connection result is being awaited, don't attempt a reconnection but instead // try to gracefully disconnect. @@ -615,7 +584,6 @@ class XmppConnection { /// Called when we timeout during connecting Future _onConnectingTimeout() async { _log.severe('Connection stuck in "connecting". Causing a reconnection...'); - await _resetIsConnectionRunning(); await handleError(TimeoutError()); } @@ -844,7 +812,6 @@ class XmppConnection { /// a disco sweep among other things. Future _onNegotiationsDone() async { // Set the connection state - await _resetIsConnectionRunning(); await _setConnectionState(XmppConnectionState.connected); // Enable reconnections @@ -880,14 +847,12 @@ class XmppConnection { // We failed before authenticating if (!_isAuthenticated) { _log.severe('No negotiator could be picked while unauthenticated'); - await _resetIsConnectionRunning(); await handleError(NoMatchingAuthenticationMechanismAvailableError()); return; } else { _log.severe( 'No negotiator could be picked while negotiations are not done', ); - await _resetIsConnectionRunning(); await handleError(NoAuthenticatorAvailableError()); return; } @@ -896,7 +861,6 @@ class XmppConnection { final result = await _currentNegotiator!.negotiate(nonza); if (result.isType()) { _log.severe('Negotiator returned an error'); - await _resetIsConnectionRunning(); await handleError(result.get()); return; } @@ -919,7 +883,6 @@ class XmppConnection { !_isNegotiationPossible(_streamFeatures)) { _log.finest('Negotiations done!'); _updateRoutingState(RoutingState.handleStanzas); - await _resetIsConnectionRunning(); await _onNegotiationsDone(); } else { _currentNegotiator = getNextNegotiator(_streamFeatures); @@ -943,7 +906,6 @@ class XmppConnection { _log.finest('Negotiations done!'); _updateRoutingState(RoutingState.handleStanzas); - await _resetIsConnectionRunning(); await _onNegotiationsDone(); } else { _log.finest('Picking new negotiator...'); @@ -962,7 +924,6 @@ class XmppConnection { ); _updateRoutingState(RoutingState.handleStanzas); - await _resetIsConnectionRunning(); await _onNegotiationsDone(); break; } @@ -1052,7 +1013,6 @@ class XmppConnection { /// To be called when we lost the network connection. Future _onNetworkConnectionLost() async { _socket.close(); - await _resetIsConnectionRunning(); await _setConnectionState(XmppConnectionState.notConnected); } @@ -1112,22 +1072,8 @@ class XmppConnection { bool enableReconnectOnSuccess = true, bool ignoreLock = false, }) async { - if (!ignoreLock) { - if (await _testAndSetIsConnectionRunning()) { - _log.fine( - 'Cancelling this connection attempt as one appears to be already running.', - ); - return Future.value( - Result( - ConnectionAlreadyRunningError(), - ), - ); - } - - await _resetIsConnectionRunning(); - } else { - _log.fine('Ignoring connection lock as ignoreLock = true'); - } + // Kill a possibly existing connection + _socket.close(); if (waitUntilLogin) { _log.finest('Setting up completer for awaiting completed login'); diff --git a/packages/moxxmpp/lib/src/connection_errors.dart b/packages/moxxmpp/lib/src/connection_errors.dart index f0db272..5b5ca61 100644 --- a/packages/moxxmpp/lib/src/connection_errors.dart +++ b/packages/moxxmpp/lib/src/connection_errors.dart @@ -4,12 +4,6 @@ import 'package:moxxmpp/src/negotiators/negotiator.dart'; /// The reason a call to `XmppConnection.connect` failed. abstract class XmppConnectionError extends XmppError {} -/// Returned by `XmppConnection.connect` when a connection is already active. -class ConnectionAlreadyRunningError extends XmppConnectionError { - @override - bool isRecoverable() => true; -} - /// Returned by `XmppConnection.connect` when a negotiator returned an unrecoverable /// error. Only returned when waitUntilLogin is true. class NegotiatorReturnedError extends XmppConnectionError { diff --git a/packages/moxxmpp/test/helpers/xmpp.dart b/packages/moxxmpp/test/helpers/xmpp.dart index 3833b8b..80c4511 100644 --- a/packages/moxxmpp/test/helpers/xmpp.dart +++ b/packages/moxxmpp/test/helpers/xmpp.dart @@ -139,6 +139,11 @@ class StubTCPSocket extends BaseSocketWrapper { Stream getEventStream() => _eventStream.stream.asBroadcastStream(); + /// "Closes" the socket unexpectedly + void injectSocketFault() { + _eventStream.add(XmppSocketClosureEvent(false)); + } + /// Let the "connection" receive [data]. void injectRawXml(String data) { // ignore: avoid_print diff --git a/packages/moxxmpp/test/xmpp_test.dart b/packages/moxxmpp/test/xmpp_test.dart index b979105..f5259df 100644 --- a/packages/moxxmpp/test/xmpp_test.dart +++ b/packages/moxxmpp/test/xmpp_test.dart @@ -427,4 +427,115 @@ void main() { true, ); }); + + test('Test losing the connection while negotiation', () async { + final fakeSocket = StubTCPSocket( + [ + StringExpectation( + "", + ''' + + + + PLAIN + + ''', + ), + StringExpectation( + "AHRlc3R1c2VyAGFiYzEyMw==", + '', + ), + StringExpectation( + "", + ''' + + + + PLAIN + + ''', + ), + StringExpectation( + "AHRlc3R1c2VyAGFiYzEyMw==", + '', + ), + StringExpectation( + "", + ''' + + + + + + ''', + ), + StanzaExpectation( + '', + 'testuser@example.org/MU29eEZn', + ignoreId: true, + ), + ], + ); + + final conn = XmppConnection( + TestingReconnectionPolicy(), + AlwaysConnectedConnectivityManager(), + fakeSocket, + ); + await conn.registerManagers([ + RosterManager(TestingRosterStateManager('', [])), + DiscoManager([]), + ]); + await conn.registerFeatureNegotiators([ + SaslPlainNegotiator(), + ResourceBindingNegotiator(), + ]); + conn.setConnectionSettings( + ConnectionSettings( + jid: JID.fromString('testuser@example.org'), + password: 'abc123', + useDirectTLS: false, + ), + ); + + final result1 = conn.connect( + waitUntilLogin: true, + ); + await Future.delayed(const Duration(seconds: 2)); + + // Inject a fault + fakeSocket.injectSocketFault(); + expect( + (await result1).isType(), + false, + ); + + // Try to connect again + final result2 = await conn.connect( + waitUntilLogin: true, + ); + expect( + fakeSocket.getState(), + 6, + ); + expect( + result2.isType(), + true, + ); + }); }