35 bug getasset returns copy of asset ident not pointer by tadeas-hejnic · Pull Request #69 · ynput/ayon-usd-resolver

tadeas-hejnic · 2026-04-13T14:19:35Z

Changelog Description

Pass the AssetIdentifier as a pointer instead of copy.

Additional review information

The builds work, so it should be all OK.

…r instead of copy

…e of return value type in getAsset

…r instead of copy

…nt-not-pointer

Copilot

Pull request overview

This PR changes the resolver cache API to return an AssetIdentifier via pointer rather than returning it by value, with the intent of avoiding copies when fetching cached assets.

Changes:

Updated ResolverContextCache::getAsset() and PinningFileHandler::getAssetData() to return AssetIdentifier*.
Updated AyonUsdResolver::_Resolve() to use the pointer-returning cache API.
Modified cache hit paths to return pointers to elements stored in the cache containers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
`src/AyonUsdResolver/resolver.cpp`	Switched to using `AssetIdentifier*` from the cache and dereferencing it to build the resolved path.
`src/AyonUsdResolver/cache/resolverContextCache.h`	Updated public API signatures to return `AssetIdentifier*`.
`src/AyonUsdResolver/cache/resolverContextCache.cpp`	Implemented pointer-based returns, including heap allocations and returning pointers to `unordered_set` elements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T08:55:53Z

@@ -76,9 +76,11 @@ PinningFileHandler::getAssetData(const std::string &resolveKey) {
        return assetEntry;
    }

+    assetEntry = new AssetIdentifier();
+
    if (!pinnedAssetPath.empty()) {
-        assetEntry.setAssetIdentifier(resolveKey);
-        assetEntry.setResolvedAssetPath(pinnedAssetPath);
+        assetEntry->setAssetIdentifier(resolveKey);
+        assetEntry->setResolvedAssetPath(pinnedAssetPath);
    }

    return assetEntry;


This function now returns a raw pointer allocated with new AssetIdentifier() but there is no corresponding ownership contract or delete, so every successful lookup leaks. Also, when the key is missing you return nullptr, which forces all callers to handle null (and resolver.cpp currently doesn't). Prefer returning by value / std::optional<AssetIdentifier> / std::unique_ptr<AssetIdentifier> (with clear ownership), or return a const AssetIdentifier&/pointer to stable storage managed by the handler.

Copilot · 2026-04-14T08:55:53Z

    if (assetIdentifier.empty()) {
-        return asset;
+        return new AssetIdentifier();
    }
    if (m_staticCache) {
        return m_pinningFileHandler->getAssetData(assetIdentifier);
    }


getAsset allocates with new AssetIdentifier() on the empty-identifier path. This leaks unless every caller deletes the returned pointer, which is not documented or implemented anywhere in the repo. If you want to avoid copies, prefer returning std::optional<std::reference_wrapper<const AssetIdentifier>>, const AssetIdentifier* to cache-owned storage, or a smart pointer that clearly expresses ownership.

Copilot · 2026-04-14T08:55:54Z

    std::shared_lock<std::shared_mutex> preCacheSharedLock(m_PreCacheSharedMutex);
    hit = m_PreCache.find(assetIdentifier);
    if (hit != m_PreCache.end()) {
-        asset = *hit;
+        asset = const_cast<AssetIdentifier*>(&(*hit)); // get the pointer without making a copy of the object
        preCacheSharedLock.unlock();

        TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
            .Msg("ResolverContextCache::getAsset: PreCache Hit on (%s) with (%s) \n",
-                 asset.getAssetIdentifier().c_str(), asset.getResolvedAssetPath().GetPathString().c_str());
+                 asset->getAssetIdentifier().c_str(), asset->getResolvedAssetPath().GetPathString().c_str());
        return asset;


Returning a non-const AssetIdentifier* to an element inside an std::unordered_set via const_cast is unsafe: callers could mutate fields that participate in hashing/equality (m_assetIdentifier), breaking container invariants and causing undefined behavior. If you need to return a pointer, make it const AssetIdentifier* (and remove the const_cast), or change the cache storage to something designed for stable, mutable values (e.g., unordered_map<std::string, AssetIdentifier>).

Copilot · 2026-04-14T08:55:54Z


        TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT).Msg("ResolverContextCache::getAsset: called ayon.resolvePath() \n");
-        this->insert(asset);
+        this->insert(*asset);


In the AYON resolve branch you call this->insert(*asset), but insert() moves from its argument (m_PreCache.insert(std::move(sourceAssetIdent))). That means the heap object *asset is left in a moved-from/empty state, yet you still return the pointer asset at the end of the function. This will cause callers to observe an empty/invalid AssetIdentifier despite having inserted the resolved entry into the cache. You likely want to either (1) insert a copy and keep returning the same object, or (2) insert and then return a pointer/reference to the cached element (not the moved-from temporary).

Suggested change

this->insert(*asset);

AssetIdentifier cachedAsset(*asset);

this->insert(cachedAsset);

Copilot · 2026-04-14T08:55:54Z

            std::shared_lock<std::shared_mutex> CommonCacheSharedLock(m_CommonCacheSharedMutex);

            TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
                .Msg("ResolverContextCache::getAsset: insert into CommonCache \n");
-            m_CommonCache.insert(asset);
+            m_CommonCache.insert(*asset);
        }


This inserts into m_CommonCache while holding only a std::shared_lock on m_CommonCacheSharedMutex. unordered_set::insert is a write operation and requires exclusive locking; doing it under a shared lock is a data race in multi-threaded use. Switch this to std::unique_lock (or reuse existing write-locking patterns used elsewhere in this file).

Copilot · 2026-04-14T08:55:55Z

         * @return AssetIdentifier with resolved path
         */
-        AssetIdentifier getAsset(const std::string &assetIdentifier, const CacheName selectedCache, const bool isAyonPath);
+        AssetIdentifier* getAsset(const std::string &assetIdentifier, const CacheName selectedCache, const bool isAyonPath);



The header change to return raw pointers (AssetIdentifier*) introduces unclear lifetime/ownership for API consumers and makes it easy to leak or dereference a dangling/null pointer. If the goal is to avoid copying, prefer returning const AssetIdentifier*/const AssetIdentifier& tied to cache lifetime, or a smart pointer/optional that makes ownership explicit.

Copilot · 2026-04-14T08:55:55Z

@@ -161,7 +161,7 @@ AyonUsdResolver::_Resolve(const std::string &assetPath) const {
            if (pos != std::string::npos) {
                sdfArgs = pathToResolve->substr(pos + cleanAssetPath.length());
            }
-            std::string resolvedPathStr = asset.getResolvedAssetPath().GetPathString() + sdfArgs;
+            std::string resolvedPathStr = asset->getResolvedAssetPath().GetPathString() + sdfArgs;
            ArResolvedPath resolvedPath(resolvedPathStr);


resolverCache->getAsset() can return nullptr (e.g., PinningFileHandler::getAssetData returns nullptr when resolveKey is not present). This code unconditionally dereferences asset (asset->getResolvedAssetPath()), which will crash for missing pinning entries or other failure paths. Consider making getAsset() never return null (e.g., return a reference/optional), or add a null check here and fall back to trying the next context / returning the original path.

tadeas-hejnic and others added 14 commits January 30, 2025 11:42

Avoid copy in getAsset: getAsset ident returns a pointer instead of copy

50fdd27

Avoid copy in getAsset: implementation of getAsset to return a pointe…

e2f1f9d

…r instead of copy

Avoid copy in getAsset: small adjustmenst in resolver caused by chang…

60cabb2

…e of return value type in getAsset

Avoid copy in getAsset: implementation of getAsset to return a pointe…

1a81fdd

…r instead of copy

Small adjustment in _Resolve, + explaining comment

79435be

resolve merge conflicts

c87c4c0

Merge branch 'develop' into 35-bug-getasset-returns-copy-of-asset-ide…

bf598ca

…nt-not-pointer

assetIdent -> AssetIdentifier

8a97620

assetIdent -> AssetIdentifier n.2

c5eb7b4

removing reference where its not needed

8b2f902

fix incorrect variable name

7af7acf

fix incorrect type name

bf6488c

fix build issues

81122a7

fix build issue

d775dab

tadeas-hejnic self-assigned this Apr 13, 2026

tadeas-hejnic added type: bug Something isn't working type: enhancement Enhancements to existing functionality and removed type: bug Something isn't working labels Apr 13, 2026

tadeas-hejnic linked an issue Apr 13, 2026 that may be closed by this pull request

[Bug] GetAsset returns Copy of Asset Ident not Pointer. #35

Open

tadeas-hejnic requested a review from philippe-ynput April 13, 2026 15:24

tadeas-hejnic marked this pull request as ready for review April 13, 2026 15:24

BigRoy requested a review from Copilot April 14, 2026 08:52

Copilot started reviewing on behalf of BigRoy April 14, 2026 08:53 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

35 bug getasset returns copy of asset ident not pointer#69

35 bug getasset returns copy of asset ident not pointer#69
tadeas-hejnic wants to merge 14 commits into
developfrom
35-bug-getasset-returns-copy-of-asset-ident-not-pointer

tadeas-hejnic commented Apr 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	this->insert(*asset);
	AssetIdentifier cachedAsset(*asset);
	this->insert(cachedAsset);

Conversation

tadeas-hejnic commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog Description

Additional review information

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tadeas-hejnic commented Apr 13, 2026 •

edited

Loading