For clients that are entering a simulator from initial login, stop executing FriendsModule.FetchFriendslist() asychronously.

Executing this asynchronously allows a race condition where subsequent friends fetches hit a cache that FetchFriendsList() had not yet populated.
Changing this to synchronous may improve issues where a user does not see friends as online even though they are.
I don't believe synchronous is a problem here, but if it is, then a more complicated signalling mechanism is required.  Locking the cache isn't sufficient.

3 files changed, 34 insertions, 11 deletions

diff --git a/OpenSim/Region/CoreModules/Avatar/Friends/FriendsModule.cs b/OpenSim/Region/CoreModules/Avatar/Friends/FriendsModule.cs
index 76d84ec..f84033b 100644
--- a/OpenSim/Region/CoreModules/Avatar/Friends/FriendsModule.cs
+++ b/OpenSim/Region/CoreModules/Avatar/Friends/FriendsModule.cs
@@ -79,9 +79,19 @@ namespace OpenSim.Region.CoreModules.Avatar.Friends
         protected IFriendsService m_FriendsService = null;
         protected FriendsSimConnector m_FriendsSimConnector;
 
-        protected Dictionary<UUID, UserFriendData> m_Friends =
-                new Dictionary<UUID, UserFriendData>();
+        /// <summary>
+        /// Cache friends lists for users.
+        /// </summary>
+        /// <remarks>
+        /// This is a complex and error-prone thing to do.  At the moment, we assume that the efficiency gained in
+        /// permissions checks outweighs the disadvantages of that complexity.
+        /// </remarks>
+        protected Dictionary<UUID, UserFriendData> m_Friends = new Dictionary<UUID, UserFriendData>();
 
+        /// <summary>
+        /// Maintain a record of viewers that need to be sent notifications for friends that are online.  This only
+        /// needs to be done on login.  Subsequent online/offline friend changes are sent by a different mechanism.
+        /// </summary>
         protected HashSet<UUID> m_NeedsListOfFriends = new HashSet<UUID>();
 
         protected IPresenceService PresenceService
@@ -189,6 +199,7 @@ namespace OpenSim.Region.CoreModules.Avatar.Friends
         {
             if (!m_Enabled)
                 return;
+
             m_log.DebugFormat("[FRIENDS MODULE]: AddRegion on {0}", Name);
 
             m_Scenes.Add(scene);
@@ -244,12 +255,23 @@ namespace OpenSim.Region.CoreModules.Avatar.Friends
             client.OnTerminateFriendship += (thisClient, agentID, exfriendID) => RemoveFriendship(thisClient, exfriendID);
             client.OnGrantUserRights += OnGrantUserRights;
 
-            Util.FireAndForget(delegate { FetchFriendslist(client); });
+            // Do not do this asynchronously.  If we do, then subsequent code can outrace FetchFriendsList() and
+            // return misleading results from the still empty friends cache.
+            // If we absolutely need to do this asynchronously, then a signalling mechanism is needed so that calls
+            // to GetFriends() will wait until FetchFriendslist() completes.  Locks are insufficient.
+            FetchFriendslist(client);
         }
 
-        /// Fetch the friends list or increment the refcount for the existing 
-        /// friends list
+
+        /// <summary>
+        /// Fetch the friends list or increment the refcount for the existing
+        /// friends list.
+        /// </summary>
+        /// <param name="client">
+        /// </param>
+        /// <returns>
         /// Returns true if the list was fetched, false if it wasn't
+        /// </returns>
         protected virtual bool FetchFriendslist(IClientAPI client)
         {
             UUID agentID = client.AgentId;
diff --git a/OpenSim/Region/CoreModules/World/Permissions/PermissionsModule.cs b/OpenSim/Region/CoreModules/World/Permissions/PermissionsModule.cs
index 3b661ed..f416f06 100644
--- a/OpenSim/Region/CoreModules/World/Permissions/PermissionsModule.cs
+++ b/OpenSim/Region/CoreModules/World/Permissions/PermissionsModule.cs
@@ -479,8 +479,7 @@ namespace OpenSim.Region.CoreModules.World.Permissions
         }
 
         protected bool IsFriendWithPerms(UUID user,UUID objectOwner)
-        {
-            
+        {            
             if (user == UUID.Zero)
                 return false;
 
diff --git a/OpenSim/Region/Framework/Scenes/EventManager.cs b/OpenSim/Region/Framework/Scenes/EventManager.cs
index 96da2c3..bf9ad65 100644
--- a/OpenSim/Region/Framework/Scenes/EventManager.cs
+++ b/OpenSim/Region/Framework/Scenes/EventManager.cs
@@ -73,8 +73,10 @@ namespace OpenSim.Region.Framework.Scenes
         /// </summary>
         public event OnNewClientDelegate OnNewClient;
 
-        public delegate void OnClientLoginDelegate(IClientAPI client);
-        public event OnClientLoginDelegate OnClientLogin;
+        /// <summary>
+        /// Fired if the client entering this sim is doing so as a new login
+        /// </summary>
+        public event Action<IClientAPI> OnClientLogin;
 
         public delegate void OnNewPresenceDelegate(ScenePresence presence);
 
@@ -651,10 +653,10 @@ namespace OpenSim.Region.Framework.Scenes
 
         public void TriggerOnClientLogin(IClientAPI client)
         {
-            OnClientLoginDelegate handlerClientLogin = OnClientLogin;
+            Action<IClientAPI> handlerClientLogin = OnClientLogin;
             if (handlerClientLogin != null)
             {
-                foreach (OnClientLoginDelegate d in handlerClientLogin.GetInvocationList())
+                foreach (Action<IClientAPI> d in handlerClientLogin.GetInvocationList())
                 {
                     try
                     {