Skip to content

fix(concurrent): count the -Xmx headroom as available memory in MemoryLimitCalculator - #7809

Merged
Aias00 merged 1 commit into
apache:masterfrom
choksi2212:fix/memory-limit-calculator-xmx-headroom
Oct 19, 2026
Merged

Aias00 merged 1 commit into
apache:masterfrom
choksi2212:fix/memory-limit-calculator-xmx-headroom

Conversation

@choksi2212

Copy link
Copy Markdown
Contributor

Describe what this PR does and why we are doing this

MemoryLimitCalculator.maxAvailable() sampled only Runtime.freeMemory(), i.e. the free part of the currently committed heap. Queue admission built on it (MemorySafeLinkedBlockingQueue, which backs MemorySafeTaskQueue, the shared HierarchicalWheelTimer queue and the work queues) therefore rejected tasks whenever the committed heap held less free memory than the configured floor, even when the JVM could still commit heap up to -Xmx.

The production-visible case from #7803: with -Xms256m -Xmx768m the wheel timer queue (built with the Constants.THE_256_MB floor) silently dropped recurring tasks such as heartbeat registrations — 156MiB free committed vs 668MiB headroom to -Xmx, accepted=false, with the default DiscardPolicy swallowing the rejection.

Change

refresh() now derives the limit from maxMemory() - (totalMemory() - freeMemory()):

  • When the committed heap already equals -Xmx (-Xms == -Xmx, the usual pinned-heap / container deployment), the value is identical to freeMemory(), so those deployments keep their exact behavior.
  • When the heap can still grow, admission only opens up by the amount the JVM can genuinely still commit toward -Xmx.

Consumers that pick this up automatically: MemorySafeLinkedBlockingQueue (+ MemorySafeTaskQueue), MemorySafeLRUMap, and the maxWorkQueueMemory default in ShenyuConfig. The apache/dubbo twin of this class still samples freeMemory(); this change intentionally improves on that, so the two are no longer byte-equivalent here.

Tests

  • New MemorySafeLinkedBlockingQueueTest.testAdmissionCountsMemoryTheHeapCanStillCommit: on a growable heap, a floor between the free committed memory and the -Xmx headroom must admit. Verified to fail on the previous behavior.
  • New MemoryLimitCalculatorTest.testMaxAvailableCountsMemoryTheHeapCanStillCommit: maxAvailable() must cover the part of the heap the JVM can still commit.
  • The existing tests that used Integer.MAX_VALUE as a "guaranteed rejection" floor now use a maxMemory() + 1 floor guarded by assumeTrue: the old floor could never force rejection once the headroom is counted on heaps above 2GiB, and was already environment-dependent before. They pass unchanged on the previous behavior.
  • shenyu-common module suite: 509 tests, 0 failures; the memory classes additionally verified under -Xms64m -Xmx256m (growable heap, all assumption-guarded tests execute). checkstyle:check passes.

Fixes #7803

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true. (Ran ./mvnw -pl shenyu-common -am test for the affected module: 509 tests, 0 failures, plus checkstyle:check; the change is confined to shenyu-common.)

…yLimitCalculator

MemoryLimitCalculator.maxAvailable() sampled only Runtime.freeMemory(), the free
part of the currently committed heap, so MemorySafeLinkedBlockingQueue and the
admission paths it backs (the shared HierarchicalWheelTimer queue and the work
queues) rejected tasks whenever the committed heap had less free space than the
configured floor, even though the JVM could still commit heap up to -Xmx. On
small-Xms deployments the wheel timer queue (Constants.THE_256_MB) then silently
dropped recurring tasks such as heartbeat registrations (issue repro:
-Xms256m -Xmx768m, 156MiB free committed vs 668MiB headroom to -Xmx -> rejected).

refresh() now derives the limit from maxMemory() - (totalMemory() - freeMemory()),
which is identical to freeMemory() when the committed heap already equals -Xmx
(pinned-heap deployments keep their exact behavior) and only admits more when the
heap can actually grow. The apache/dubbo twin of this class still samples
freeMemory(), so this intentionally improves on it.

The MemorySafeLinkedBlockingQueue tests now use a maxMemory()+1 floor guarded by
an assumption instead of the Integer.MAX_VALUE floor, which could no longer force
rejection on heaps above 2GiB once the headroom is counted, and adds regression
tests for the headroom accounting.

Fixes apache#7803

Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
@choksi2212

Copy link
Copy Markdown
Contributor Author

Hi maintainers — this is my first PR to the repo, so the pull_request workflows (ci, it, e2e, CodeQL and friends) are showing action_required and haven't been able to run yet. Could someone approve the workflow runs when convenient? No rush.

Everything is verified locally in the meantime: the 9 MemoryLimitCalculator tests pass on a growable heap (-Xms64m -Xmx256m), the full shenyu-common module suite is green (509 tests, 0 failures), and checkstyle:check passes. The two new regression tests also fail on pristine master with the exact behaviour from #7803, so the fix is load-bearing.

Thanks!

@BobSong-dev BobSong-dev 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.

@Aias00

Thanks for the fix and the regression tests. The headroom calculation addresses the small-initial-heap case in my local verification. Could we make this assertion independent of the background refresh timing?

maxAvailable() is a cached sample refreshed every 50 ms, whereas runtime.totalMemory() is read live. With -Xms64m -Xmx256m, I initialized the calculator while about 170 MiB was used, released those objects, and triggered GC. The heap shrank to 64 MiB, so this comparison became 85 MiB >= 192 MiB and the new test method failed. It passed again after the cache refreshed; I reproduced this in five independent runs.

The formula itself is correct in this scenario. Could we test it against a controlled memory snapshot so a GC between sampling and assertion cannot make the regression test fail spuriously?

@Aias00 Aias00 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.

Approved. The arithmetic is right: maxMemory - (totalMemory - freeMemory) is exactly freeMemory + (maxMemory - totalMemory), i.e. the free part of the committed heap plus the headroom the JVM can still commit up to -Xmx. The inline comment is also accurate that it degenerates to the old freeMemory() when the committed heap already equals -Xmx (-Xms == -Xmx), which is what makes the change safe for existing configurations.

The new testAdmissionCountsMemoryTheHeapCanStillCommit is the valuable part of this PR. Choosing floor = (free + headroom) / 2 places it strictly between the old and the new answer, so the test actually distinguishes the two implementations instead of passing trivially — that is the test the previous suite was missing, and it is why the old formula shipped.

Two notes, neither blocking:

  1. This loosens the admission margin, deliberately. MemoryLimitCalculator.maxAvailable() and defaultLimit() are consumed by MemoryLimiter and the memory-safe queue, and maxAvailable is now strictly larger than before. Callers that were conservatively rejecting work near -Xmx will now admit it, which is the point of the fix, but it does mean the safety margin is now “everything up to -Xmx” rather than “what is already committed and free”. defaultLimit() keeps the 80% factor, so the effective ceiling moves from 80% of free-committed to 80% of free+headroom. Worth stating in the release notes, because operators tune pool sizes against the old number.

  2. The new assertion in MemoryLimitCalculatorTest is close to tautological. maxAvailable() >= maxMemory - totalMemory holds for any value that includes the headroom, and the old freeMemory() implementation would only fail it when freeMemory() < maxMemory - totalMemory. It is a directional guard rather than a real assertion; the queue-level test above is doing the actual work.

The assumeTrue guards and the switch from Integer.MAX_VALUE to maxMemory() + 1 in MemorySafeLinkedBlockingQueueTest are a good cleanup — the old “all memory reserved” fixtures were only deterministic on heaps below 2 GiB.

Note: CI has not reported on this branch yet — please confirm the workflow passes before merge.

@Aias00
Aias00 merged commit 65730ff into apache:master Oct 19, 2026
51 checks passed
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.

[BUG] Timer queue rejects tasks with a small initial heap despite available max heap

3 participants