[PATCH] Fix some concurrency issues regarding "inventory" asset transfers.

Hello again! When requesting an inventory asset via UDP using the RequestInventoryAsset method, a packet is sent out and once a result is obtained from the asset server, the AssetReceivedCallback callback is used to process the result. The AssetReceivedCallback is raised with two objects: a transfer object and an asset object. The asset object contains the asset, if the retrieval of the asset was successful and the transfer object contains the the Success parameter in order to check if the transfer was successful.

Within the AssetReceivedCallback a user is probably expected to check transfer.Success in order to verify whether the asset transfer was successful. However, that poses a major problem when multiple requests for asset transfers are in effect concurrently because the user cannot really distinguish which asset was requested given that multiple callbacks may be raised at the same time concurrently. In order to verify this, and up to now, a common pattern is to check whether the asset UUID matches the UUID of the asset that has been requested initially by the user when calling the RequestInventoryAsset method.

In other words, the following pattern may be common (extracted from CreateNotecardCommand.cs contained in the TestClient example client):

Client.Assets.RequestInventoryAsset(assetID, itemID, UUID.Zero, Client.Self.AgentID, AssetType.Notecard, true,
    delegate (AssetDownload transfer, Asset asset)
    {
        if (transfer.Success)
            notecardData = transfer.AssetData;
        else
            error = transfer.Status.ToString();
        assetDownloadEvent.Set();
    }

and, in case multiple assets were requested, then the wrong asset may be inferred by the rest of the code depending on the asset transfer. One could make the following change to ensure that the correct asset is the one that the callback refers to:

Client.Assets.RequestInventoryAsset(assetID, itemID, UUID.Zero, Client.Self.AgentID, AssetType.Notecard, true,
    delegate (AssetDownload transfer, Asset asset)
    {
        if (asset != null && asset.AssetID != assetID)
            return;
        if (transfer.Success)
            notecardData = transfer.AssetData;
        else
            error = transfer.Status.ToString();
        assetDownloadEvent.Set();
    }

This would at least ensure that the callback returns the asset that was initially requested and, if multiple requests for the same asset have been made on a different thread, then so be it because the assets with asset UUIDs are immutable with regards to asset transfers. In other words, if you download the same asset a number of times, who cares if your callback is looking for an asset with a distinguishing UUID amongst the callback responses.

However, a better way to ensure that the callback matches the initial transfer request would be to allow for the random UUID to be passed by the user. As the current code stands, the RequestInventoryAsset method already generates a random UUID in order to create the AssetDownload object but the identifier is useless unless the user can access the randomly generated UUID before the callback is raised.

The following patch adds a transfer UUID that is passed by reference to the RequestInventoryAsset such that the UUID can then be referred to from within the callback function. In other words, working with the example above, the new pattern would be similar to:

var transferID = UUID.Random();
Client.Assets.RequestInventoryAsset(assetID, itemID, UUID.Zero, Client.Self.AgentID, AssetType.Notecard, true, transferID,
    delegate (AssetDownload transfer, Asset asset)
    {
        // The transfer was successful AND this is the callback for the transfer that has been requested.
        if (transfer.Success && transfer.ID == transferID)
            notecardData = transfer.AssetData;
        else
            error = transfer.Status.ToString();
        assetDownloadEvent.Set();
    }

in which case, even testing for the same asset UUID is not necessary.

One particular case that has an issue with regards to checking whether the callback corresponds to the asset being requested is in the case a user decides to request an asset such as an LSL script from within an in-world object - this case is covered by the API using RequestInventoryAsset and by passing a zero UUID as the asset UUID. When the callback is raised, there is no way to check whether the returned asset UUID matches the UUID requested because the LSL script within an object does not have an asset UUID. The result is that in case multiple script assets are requested from different objects, the callbacks will not be able to distinguish which script has been transferred; at best, the callbacks would be able to tell that the transfer completed successfully by checking transfer.Success.

From recollection, there are several other callback methods (ie, searching for UUID.Random() reveals RequestAsset, RequestUpload), that do not pass a request identifier back to the user but most times it is just sufficient to check whether the asset UUIDs match those requested. This patch addresses the issue just for the RequestInventoryAsset method and suggests passing a user-generated UUID to RequestInventoryAsset that can then be used by the user within the callback to distinguish between transfers. This would ensure that the transfer UUID is known to the user before even making the request via RequestInventoryAsset.

If useful, please apply!

cinderblocks / libremetaverse

[PATCH] Fix some concurrency issues regarding "inventory" asset transfers. #28