BF: Direct rooms can be lost on an initial /sync

element-hq/element-ios/issues/1983
matrix-org · Aug 17, 2018 · 2496e23 · 2496e23
1 parent 177af49
commit 2496e23
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 25 deletions.
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -2,7 +2,11 @@ Changes in Matrix iOS SDK in 0.11.1 (2018-08-)
 ===============================================
 
 Improvements:
-* Tests: Add DirectRoomTests
+* Tests: Add DirectRoomTests to test direct rooms management.
+
+Bug fix:
+ * Direct rooms can be lost on an initial /sync (vector-im/riot-ios/issues/1983).
+ * Fix possible race conditions in direct rooms management.
 
 Changes in Matrix iOS SDK in 0.11.0 (2018-08-10)
 ===============================================

diff --git a/MatrixSDK/MXSession.m b/MatrixSDK/MXSession.m
@@ -142,11 +142,6 @@ The list of global events listeners (`MXSessionEventListener`).
      */
     BOOL firstSyncDone;
 
-    /**
-     Tell whether the direct rooms list has been updated during last account data parsing.
-     */
-    BOOL didDirectRoomsChange;
-
     /**
      Queue of requested direct room change operations ([MXSession setRoom:directWithUserId:]
      or [MXSession uploadDirectRooms:])
@@ -193,7 +188,6 @@ - (id)initWithMatrixRestClient:(MXRestClient*)mxRestClient
         publicisedGroupsByUserId = [[NSMutableDictionary alloc] init];
 
         firstSyncDone = NO;
-        didDirectRoomsChange = NO;
 
         id<MXBackgroundModeHandler> handler = [MXSDKOptions sharedInstance].backgroundModeHandler;
         if (handler)
@@ -928,6 +922,12 @@ - (void)serverSyncWithServerTimeout:(NSUInteger)serverTimeout
             nextServerTimeout = 0;
         }
 
+        // Handle top-level account data
+        if (syncResponse.accountData)
+        {
+            [self handleAccountData:syncResponse.accountData];
+        }
+
         // Handle first joined rooms
         for (NSString *roomId in syncResponse.rooms.join)
         {
@@ -1048,22 +1048,6 @@ - (void)serverSyncWithServerTimeout:(NSUInteger)serverTimeout
         // and their /sync response has been processed
         [self preloadRoomsData:[self roomsInSyncResponse:syncResponse] onComplete:^{
 
-            // Handle top-level account data
-            self->didDirectRoomsChange = NO;
-            if (syncResponse.accountData)
-            {
-                [self handleAccountData:syncResponse.accountData];
-            }
-
-            if (self->didDirectRoomsChange)
-            {
-                self->didDirectRoomsChange = NO;
-
-                [[NSNotificationCenter defaultCenter] postNotificationName:kMXSessionDirectRoomsDidChangeNotification
-                                                                    object:self
-                                                                  userInfo:nil];
-            }
-
             if (self.crypto)
             {
                 // Handle device list updates
@@ -1391,7 +1375,9 @@ - (void)handleAccountData:(NSDictionary*)accountDataUpdate
                     _directRooms = directRooms;
 
                     // Update the information of the direct rooms.
-                    didDirectRoomsChange = YES;
+                    [[NSNotificationCenter defaultCenter] postNotificationName:kMXSessionDirectRoomsDidChangeNotification
+                                                                        object:self
+                                                                      userInfo:nil];
                 }
             }
 

diff --git a/MatrixSDKTests/MXLazyLoadingTests.m b/MatrixSDKTests/MXLazyLoadingTests.m
@@ -1079,7 +1079,7 @@ - (void)checkDirectRoomWithLazyLoading:(BOOL)lazyLoading
     [self createScenarioWithLazyLoading:lazyLoading readyToTest:^(MXSession *aliceSession, MXSession *bobSession, MXSession *charlieSession, NSString *roomId, XCTestExpectation *expectation) {
 
         __block id observer;
-        observer = [[NSNotificationCenter defaultCenter] addObserverForName:kMXSessionDirectRoomsDidChangeNotification object:bobSession queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *note) {
+        observer = [[NSNotificationCenter defaultCenter] addObserverForName:kMXSessionDirectRoomsDidChangeNotification object:aliceSession queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *note) {
 
             // Wait until we get the 3 direct rooms
             if (observer
@@ -1101,9 +1101,28 @@ - (void)checkDirectRoomWithLazyLoading:(BOOL)lazyLoading
                     filter = [MXFilterJSONModel syncFilterForLazyLoading];
                 }
 
+                __block NSUInteger directRoomsDidChangeNotificationCountWhileStarting = 0;
+                [[NSNotificationCenter defaultCenter] addObserverForName:kMXSessionDirectRoomsDidChangeNotification object:aliceSession2 queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *note) {
+
+                    directRoomsDidChangeNotificationCountWhileStarting++;
+
+                    // We must receive a single big update
+                    // Else we have a race condition like in https://github.com/vector-im/riot-ios/issues/1983
+                    XCTAssertEqual(aliceSession2.directRooms.count, 2);
+                    XCTAssertEqual(aliceSession2.directRooms[bobSession.myUser.userId].count, 2);
+                    XCTAssertEqual(aliceSession2.directRooms[charlieSession.myUser.userId].count, 1);
+
+                    if (directRoomsDidChangeNotificationCountWhileStarting > 1)
+                    {
+                        XCTFail(@"We should receive one big kMXSessionDirectRoomsDidChangeNotification");
+                    }
+                }];
+
+
                 [aliceSession2 startWithSyncFilter:filter onServerSyncDone:^{
 
                     // -> He must still see 2 direct rooms with Alice and 1 with Charlie
+                    XCTAssertEqual(aliceSession2.directRooms.count, 2);
                     XCTAssertEqual(aliceSession2.directRooms[bobSession.myUser.userId].count, 2);
                     XCTAssertEqual(aliceSession2.directRooms[charlieSession.myUser.userId].count, 1);
                     [expectation fulfill];