Skip to content

fix: Allocate TaggedCache::getKeys() memory outside of lock#7567

Open
ximinez wants to merge 3 commits into
developfrom
ximinez/fix-getkeys
Open

fix: Allocate TaggedCache::getKeys() memory outside of lock#7567
ximinez wants to merge 3 commits into
developfrom
ximinez/fix-getkeys

Conversation

@ximinez

@ximinez ximinez commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

High Level Overview of Change

Moves the allocation of storage for the keys vector in TaggedCache::getKeys() outside of lock. Because the size of the cache may change while allocating, uses a loop to check (under lock) that the capacity is sufficient before filling.

Context of Change

Aside from the fact that allocating memory under a lock is almost always Wrong, there is the specific potential for excess lock contention if a given cache is very large at the time that getKeys() is called. Such an allocation could take a significant amount of time, causing other threads to stall waiting for it.

The primary consumer of getKeys() is online_delete database rotation, which is supposed to be a lower priority than most other functions, deferring if they fall behind. (See also #5531.)

ximinez added 2 commits June 17, 2026 13:06
- Uses a loop in case the size grows while the lock is free. Guarantees
  the result vector will not need to allocate under lock.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One narrowing conversion bug flagged inline — see line 541.

Review by Claude Sonnet 4.6 · Prompt: V15

Comment thread include/xrpl/basics/TaggedCache.ipp Outdated
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.0%. Comparing base (480676d) to head (0542847).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7567   +/-   ##
=======================================
  Coverage     82.0%   82.0%           
=======================================
  Files         1007    1007           
  Lines        76876   76880    +4     
  Branches      8981    8981           
=======================================
+ Hits         63001   63005    +4     
  Misses       13866   13866           
  Partials         9       9           
Files with missing lines Coverage Δ
include/xrpl/basics/TaggedCache.ipp 86.5% <100.0%> (+0.2%) ⬆️

... and 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant