From 4fc0cfba3ce1e6545e334f8e34a0e5b45274081e Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 25 Sep 2012 21:35:39 +0100 Subject: 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) --- .../AssetTransaction/AgentAssetsTransactions.cs | 123 +++++++-------------- 1 file changed, 38 insertions(+), 85 deletions(-) (limited to 'OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs') 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 } /// - /// Return a xfer uploader if one does not already exist. + /// Return the xfer uploader for the given transaction. /// + /// + /// If an uploader does not already exist for this transaction then it is created, otherwise the existing + /// uploader is returned. + /// /// - /// - /// 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. - /// - /// - /// 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. - /// - public AssetXferUploader RequestXferUploader(UUID transactionID, UUID assetID) + /// The asset xfer uploader + 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); } /// @@ -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 -- cgit v1.1 From 2f795e4fa6433269748f4e062d4bba7197e46ab1 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 25 Sep 2012 22:08:11 +0100 Subject: Move UDP update task item code to AssetXferUploader to match existing create user item and update user item mechanisms This is done for consistency and to allow removal or some access methods that increase code complexity. However, this path has not been used for a long time, not even by LL 1.23 - viewers use caps http upload for this instead --- .../AssetTransaction/AgentAssetsTransactions.cs | 47 +--------------------- 1 file changed, 2 insertions(+), 45 deletions(-) (limited to 'OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs') diff --git a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs index bba7b9c..59d0075 100644 --- a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs +++ b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs @@ -155,56 +155,13 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction description, name, invType, type, wearableType, nextOwnerMask); } - /// - /// Get an uploaded asset. If the data is successfully retrieved, - /// the transaction will be removed. - /// - /// - /// The asset if the upload has completed, null if it has not. - private AssetBase GetTransactionAsset(UUID transactionID) - { - lock (XferUploaders) - { - if (XferUploaders.ContainsKey(transactionID)) - { - AssetXferUploader uploader = XferUploaders[transactionID]; - AssetBase asset = uploader.GetAssetData(); - RemoveXferUploader(transactionID); - - return asset; - } - } - - return null; - } - public void RequestUpdateTaskInventoryItem(IClientAPI remoteClient, SceneObjectPart part, UUID transactionID, TaskInventoryItem item) { - 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; + AssetXferUploader uploader = RequestXferUploader(transactionID); - 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); - } + uploader.RequestUpdateTaskInventoryItem(remoteClient, transactionID, item); } public void RequestUpdateInventoryItem(IClientAPI remoteClient, -- cgit v1.1 From eb5bec96e49466ede4ece376cb15cd68477ce983 Mon Sep 17 00:00:00 2001 From: Justin Clark-Casey (justincc) Date: Tue, 25 Sep 2012 22:54:20 +0100 Subject: Insert transaction ID into AssetXferUploader constructor rather than at UploadAsset() to prevent item creation failure when NewInventoryItem thread reachs the object first. This was preventing the previous race condition fix in 4fc0cfb from actually working. This commit also removes some of the pointless transaction id checks - these conditions are already being enforced in AgentAssetsTransactions. --- .../CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs') diff --git a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs index 59d0075..0271738 100644 --- a/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs +++ b/OpenSim/Region/CoreModules/Agent/AssetTransaction/AgentAssetsTransactions.cs @@ -73,7 +73,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction { if (!XferUploaders.ContainsKey(transactionID)) { - uploader = new AssetXferUploader(this, m_Scene, m_dumpAssetsToFile); + uploader = new AssetXferUploader(this, m_Scene, transactionID, m_dumpAssetsToFile); // m_log.DebugFormat( // "[AGENT ASSETS TRANSACTIONS]: Adding asset xfer uploader {0} since it didn't previously exist", transactionID); @@ -151,7 +151,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction AssetXferUploader uploader = RequestXferUploader(transactionID); uploader.RequestCreateInventoryItem( - remoteClient, transactionID, folderID, callbackID, + remoteClient, folderID, callbackID, description, name, invType, type, wearableType, nextOwnerMask); } @@ -161,7 +161,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction { AssetXferUploader uploader = RequestXferUploader(transactionID); - uploader.RequestUpdateTaskInventoryItem(remoteClient, transactionID, item); + uploader.RequestUpdateTaskInventoryItem(remoteClient, item); } public void RequestUpdateInventoryItem(IClientAPI remoteClient, @@ -169,7 +169,7 @@ namespace OpenSim.Region.CoreModules.Agent.AssetTransaction { AssetXferUploader uploader = RequestXferUploader(transactionID); - uploader.RequestUpdateInventoryItem(remoteClient, transactionID, item); + uploader.RequestUpdateInventoryItem(remoteClient, item); } } } \ No newline at end of file -- cgit v1.1