Fix occasional race condition failure when creating new clothing/body parts in the viewer or updating existing assets.

On creating these items, the viewer sends a UDP AssetUploadRequest followed by a CreateInventoryItem.
It was possible for the CreateInventoryItem/UpdateInventoryItem to occasionally outrace the AssetUploadRequest and fail to find an initialized Xfer object, at which point the item create would fail.
So instead we always set up a Xfer object on either the asset or inventory item update request.
This does not introduce a new race because code already exists to delay the item operation until the asset is uploaded if necessary (but this only worked if the xfer object already existed)

3 files changed, 93 insertions, 110 deletions

diff --git a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs
index b557ffe..bba7b9c 100644
--- a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs
+++ b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs
@@ -57,39 +57,36 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         }
 
         /// <summary>
-        /// Return a xfer uploader if one does not already exist.
+        /// Return the xfer uploader for the given transaction.
         /// </summary>
+        /// <remarks>
+        /// If an uploader does not already exist for this transaction then it is created, otherwise the existing
+        /// uploader is returned.
+        /// </remarks>
         /// <param name="transactionID"></param>
-        /// <param name="assetID">
-        /// We must transfer the new asset ID into the uploader on creation, otherwise
-        /// we can see race conditions with other threads which can retrieve an item before it is updated with the new
-        /// asset id.
-        /// </param>
-        /// <returns>
-        /// The xfer uploader requested.  Null if one is already in existence.
-        /// FIXME: This is a bizarre thing to do, and is probably meant to signal an error condition if multiple
-        /// transfers are made.  Needs to be corrected.
-        /// </returns>
-        public AssetXferUploader RequestXferUploader(UUID transactionID, UUID assetID)
+        /// <returns>The asset xfer uploader</returns>
+        public AssetXferUploader RequestXferUploader(UUID transactionID)
         {
+            AssetXferUploader uploader;
+
             lock (XferUploaders)
             {
                 if (!XferUploaders.ContainsKey(transactionID))
                 {
-                    AssetXferUploader uploader = new AssetXferUploader(this, m_Scene, assetID, m_dumpAssetsToFile);
+                    uploader = new AssetXferUploader(this, m_Scene, m_dumpAssetsToFile);
 
 //                    m_log.DebugFormat(
 //                        "[AGENT ASSETS TRANSACTIONS]: Adding asset xfer uploader {0} since it didn't previously exist", transactionID);
 
                     XferUploaders.Add(transactionID, uploader);
-
-                    return uploader;
+                }
+                else
+                {
+                    uploader = XferUploaders[transactionID];
                 }
             }
 
-            m_log.WarnFormat("[AGENT ASSETS TRANSACTIONS]: Ignoring request for asset xfer uploader {0} since it already exists", transactionID);
-
-            return null;
+            return uploader;
         }
 
         public void HandleXfer(ulong xferID, uint packetID, byte[] data)
@@ -151,23 +148,11 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
                 string description, string name, sbyte invType,
                sbyte type, byte wearableType, uint nextOwnerMask)
         {
-            AssetXferUploader uploader = null;
+            AssetXferUploader uploader = RequestXferUploader(transactionID);
 
-            lock (XferUploaders)
-            {
-                if (XferUploaders.ContainsKey(transactionID))
-                    uploader = XferUploaders[transactionID];
-            }
-
-            if (uploader != null)
-                uploader.RequestCreateInventoryItem(
-                    remoteClient, transactionID, folderID,
-                    callbackID, description, name, invType, type,
-                    wearableType, nextOwnerMask);
-            else
-                m_log.ErrorFormat(
-                    "[AGENT ASSET TRANSACTIONS]: Could not find uploader with transaction ID {0} when handling request to create inventory item {1} from {2}",
-                    transactionID, name, remoteClient.Name);
+            uploader.RequestCreateInventoryItem(
+                remoteClient, transactionID, folderID, callbackID,
+                description, name, invType, type, wearableType, nextOwnerMask);
         }
 
         /// <summary>
@@ -197,69 +182,37 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
                 SceneObjectPart part, UUID transactionID,
                 TaskInventoryItem item)
         {
-            AssetXferUploader uploader = null;
+            AssetBase asset = GetTransactionAsset(transactionID);
 
-            lock (XferUploaders)
-            {
-                if (XferUploaders.ContainsKey(transactionID))
-                    uploader = XferUploaders[transactionID];
-            }
+            // Only legacy viewers use this, and they prefer CAPS, which
+            // we have, so this really never runs.
+            // Allow it, but only for "safe" types.
+            if ((InventoryType)item.InvType != InventoryType.Notecard &&
+                (InventoryType)item.InvType != InventoryType.LSL)
+                return;
 
-            if (uploader != null)
+            if (asset != null)
             {
-                AssetBase asset = GetTransactionAsset(transactionID);
-
-                // Only legacy viewers use this, and they prefer CAPS, which 
-                // we have, so this really never runs.
-                // Allow it, but only for "safe" types.
-                if ((InventoryType)item.InvType != InventoryType.Notecard &&
-                    (InventoryType)item.InvType != InventoryType.LSL)
-                    return;
-
-                if (asset != null)
-                {
 //                    m_log.DebugFormat(
 //                        "[AGENT ASSETS TRANSACTIONS]: Updating item {0} in {1} for transaction {2}", 
 //                        item.Name, part.Name, transactionID);
-                    
-                    asset.FullID = UUID.Random();
-                    asset.Name = item.Name;
-                    asset.Description = item.Description;
-                    asset.Type = (sbyte)item.Type;
-                    item.AssetID = asset.FullID;
-
-                    m_Scene.AssetService.Store(asset);
-                }
-            }
-            else
-            {
-                m_log.ErrorFormat(
-                    "[AGENT ASSET TRANSACTIONS]: Could not find uploader with transaction ID {0} when handling request to update task inventory item {1} in {2}",
-                    transactionID, item.Name, part.Name);
+                
+                asset.FullID = UUID.Random();
+                asset.Name = item.Name;
+                asset.Description = item.Description;
+                asset.Type = (sbyte)item.Type;
+                item.AssetID = asset.FullID;
+
+                m_Scene.AssetService.Store(asset);
             }
         }
 
         public void RequestUpdateInventoryItem(IClientAPI remoteClient,
                 UUID transactionID, InventoryItemBase item)
         {
-            AssetXferUploader uploader = null;
-
-            lock (XferUploaders)
-            {
-                if (XferUploaders.ContainsKey(transactionID))
-                    uploader = XferUploaders[transactionID];
-            }
+            AssetXferUploader uploader = RequestXferUploader(transactionID);
 
-            if (uploader != null)
-            {
-                uploader.RequestUpdateInventoryItem(remoteClient, transactionID, item);
-            }
-            else
-            {
-                m_log.ErrorFormat(
-                    "[AGENT ASSET TRANSACTIONS]: Could not find uploader with transaction ID {0} when handling request to update inventory item {1} for {2}",
-                    transactionID, item.Name, remoteClient.Name);
-            }
+            uploader.RequestUpdateInventoryItem(remoteClient, transactionID, item);
         }
     }
-}
+}
\ No newline at end of file
diff --git a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetTransactionModule.cs b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetTransactionModule.cs
index 7081989..10a0794 100644
--- a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetTransactionModule.cs
+++ b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetTransactionModule.cs
@@ -274,13 +274,8 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
             }
 
             AgentAssetTransactions transactions = GetUserTransactions(remoteClient.AgentId);
-            AssetXferUploader uploader = transactions.RequestXferUploader(transaction, assetID);
-
-            if (uploader != null)
-            {
-                uploader.Initialise(remoteClient, assetID, transaction, type,
-                        data, storeLocal, tempFile);
-            }
+            AssetXferUploader uploader = transactions.RequestXferUploader(transaction);
+            uploader.StartUpload(remoteClient, assetID, transaction, type, data, storeLocal, tempFile);
         }
 
         /// <summary>
diff --git a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetXferUploader.cs b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetXferUploader.cs
index 4cedfe6..d134c43 100644
--- a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetXferUploader.cs
+++ b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AssetXferUploader.cs
@@ -49,11 +49,26 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         private static readonly ILog m_log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);
 
         /// <summary>
+        /// Upload state.
+        /// </summary>
+        /// <remarks>
+        /// New -> Uploading -> Complete
+        /// </remarks>
+        private enum UploadState
+        {
+            New,
+            Uploading,
+            Complete
+        }
+
+        /// <summary>
         /// Reference to the object that holds this uploader.  Used to remove ourselves from it's list if we
         /// are performing a delayed update.
         /// </summary>
         AgentAssetTransactions m_transactions;
 
+        private UploadState m_uploadState = UploadState.New;
+
         private AssetBase m_asset;
         private UUID InventFolder = UUID.Zero;
         private sbyte invType = 0;
@@ -65,7 +80,6 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
 
         private string m_description = String.Empty;
         private bool m_dumpAssetToFile;
-        private bool m_finished = false;
         private string m_name = String.Empty;
         private bool m_storeLocal;
         private uint nextPerm = 0;
@@ -77,11 +91,10 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         public ulong XferID;
         private Scene m_Scene;
 
-        public AssetXferUploader(AgentAssetTransactions transactions, Scene scene, UUID assetID, bool dumpAssetToFile)
+        public AssetXferUploader(AgentAssetTransactions transactions, Scene scene, bool dumpAssetToFile)
         {
             m_transactions = transactions;
             m_Scene = scene;
-            m_asset = new AssetBase() { FullID = assetID };
             m_dumpAssetToFile = dumpAssetToFile;
         }
 
@@ -127,20 +140,43 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         }
 
         /// <summary>
-        /// Initialise asset transfer from the client
+        /// Start asset transfer from the client
         /// </summary>
-        /// <param name="xferID"></param>
-        /// <param name="packetID"></param>
-        /// <param name="data"></param>
-        public void Initialise(IClientAPI remoteClient, UUID assetID,
-                UUID transaction, sbyte type, byte[] data, bool storeLocal,
-                bool tempFile)
+        /// <param name="remoteClient"></param>
+        /// <param name="assetID"></param>
+        /// <param name="transaction"></param>
+        /// <param name="type"></param>
+        /// <param name="data">
+        /// Optional data.  If present then the asset is created immediately with this data
+        /// rather than requesting an upload from the client.  The data must be longer than 2 bytes.
+        /// </param>
+        /// <param name="storeLocal"></param>
+        /// <param name="tempFile"></param>
+        public void StartUpload(
+            IClientAPI remoteClient, UUID assetID, UUID transaction, sbyte type, byte[] data, bool storeLocal,
+            bool tempFile)
         {
 //            m_log.DebugFormat(
 //                "[ASSET XFER UPLOADER]: Initialised xfer from {0}, asset {1}, transaction {2}, type {3}, storeLocal {4}, tempFile {5}, already received data length {6}",
 //                remoteClient.Name, assetID, transaction, type, storeLocal, tempFile, data.Length);
 
+            lock (this)
+            {
+                if (m_uploadState != UploadState.New)
+                {
+                    m_log.WarnFormat(
+                        "[ASSET XFER UPLOADER]: Tried to start upload of asset {0}, transaction {1} for {2} but this is already in state {3}.  Aborting.",
+                        assetID, transaction, remoteClient.Name, m_uploadState);
+
+                    return;
+                }
+
+                m_uploadState = UploadState.Uploading;
+            }
+
             ourClient = remoteClient;
+
+            m_asset = new AssetBase() { FullID = assetID };
             m_asset.Name = "blank";
             m_asset.Description = "empty";
             m_asset.Type = type;
@@ -175,14 +211,14 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
 
         protected void SendCompleteMessage()
         {
-            ourClient.SendAssetUploadCompleteMessage(m_asset.Type, true,
-                    m_asset.FullID);
-
             // We must lock in order to avoid a race with a separate thread dealing with an inventory item or create
             // message from other client UDP.
             lock (this)
             {
-                m_finished = true;
+                m_uploadState = UploadState.Complete;
+
+                ourClient.SendAssetUploadCompleteMessage(m_asset.Type, true, m_asset.FullID);
+
                 if (m_createItem)
                 {
                     DoCreateItem(m_createItemCallback);
@@ -252,7 +288,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
                 // We must lock to avoid a race with a separate thread uploading the asset.
                 lock (this)
                 {
-                    if (m_finished)
+                    if (m_uploadState == UploadState.Complete)
                     {
                         DoCreateItem(callbackID);
                     }
@@ -280,7 +316,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
                 item.AssetID = m_asset.FullID;
                 m_Scene.InventoryService.UpdateItem(item);
 
-                if (m_finished)
+                if (m_uploadState == UploadState.Complete)
                 {
                     StoreAssetForItemUpdate(item);
                 }
@@ -416,7 +452,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         /// <returns>null if the asset has not finished uploading</returns>
         public AssetBase GetAssetData()
         {
-            if (m_finished)
+            if (m_uploadState == UploadState.Complete)
             {
                 ValidateAssets();
                 return m_asset;
@@ -469,4 +505,3 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction
         }
     }
 }
-