Skip to content

fix: release concurrency slots on downstream limit exceptions - #16456

Open
beemines wants to merge 2 commits into
apache:3.3from
beemines:fix/limit-filter-slot-release
Open

beemines wants to merge 2 commits into
apache:3.3from
beemines:fix/limit-filter-slot-release

Conversation

@beemines

@beemines beemines commented Sep 8, 2026

Copy link
Copy Markdown

What is the purpose of the change?

Fixes #16455.

ActiveLimitFilter and ExecuteLimitFilter skip error accounting for every LIMIT_EXCEEDED_EXCEPTION. If a downstream invoker throws that exception after admission, or completes its result future exceptionally with it, the acquired concurrency slot is never released. At a limit of 1, later calls can time out or be rejected despite no call remaining in flight.

Track whether each filter acquired a slot during the current invocation attempt. Only locally rejected limit exceptions skip accounting; downstream limit exceptions release the slot through the existing error callback. Scope each marker by the target URL identity (also used by RpcStatus), since forking calls can share an Invocation across providers. Reset the marker on entry so sequential reuse does not reuse a previous admission decision. Existing timing, wakeup, successful-response, and non-limit-error handling are preserved.

The regression tests use the production CopyOfFilterChainNode and CallbackRegistrationInvoker lifecycle with local mock invokers and controllable futures. For both filters they verify synchronous downstream rejection, asynchronous exceptional completion, local rejection while another call remains active, sequential invocation reuse, and target isolation when two invokers share the same Invocation.

Verification

JDK 17.0.17, Maven 3.9.4, Windows; the project's Java 8 artifact target is unchanged.

./mvnw -B -pl dubbo-cluster -am \
  -Dtest=LimitFilterSlotReleaseTest,ActiveLimitFilterTest,ExecuteLimitFilterTest,RpcStatusTest,DefaultFilterChainBuilderTest \
  -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -B -pl dubbo-cluster -am -Pcheckstyle \
  -Dcheckstyle_unix.skip=true '-DspotlessFiles=.*Limit.*java' validate spotless:check
  • Before the fix on dab47b7843: 8 new regression cases, 4 assertion failures, 0 errors. Both filters retain an active count of 1 after synchronous/asynchronous downstream limit errors. The local-rejection controls pass.
  • After the final fix and coverage follow-up: all 28 selected tests pass, including 12 new cases; 0 failures, errors, or skips. These include target isolation for a shared Invocation and non-limit RPC timeout handling for both filters.
  • Java Checkstyle reports 0 violations across the selected reactor; Spotless reports no changes needed for the checked limit-related files, and git diff --check is clean.

On this Windows checkout, the initial Unix resource-line-ending check reports 59 pre-existing CRLF resource files due to core.autocrlf=true. The separate Unix check was disabled locally for subsequent validation; no unrelated resources were modified. All three changed Java files use LF. The final test run also enabled Java Checkstyle and applied Spotless without further changes.

The regression targets exceptions delivered to onError, not business exceptions carried inside a normally completed AppResponse. No registry or external RPC service is required. The full repository test suite was not run.

AI assistance was used for investigation, code, and tests. The failing behavior was reproduced locally before applying the production fix.

Coverage follow-up

Added two non-limit RPC timeout cases (consumer and provider filters). Each verifies that the original exception propagates, the admitted slot is released, failure accounting increments, and the next invocation succeeds. Production code is unchanged by this follow-up.

The local JaCoCo 0.8.15 report, generated from the same 28 selected tests using CI's jacoco,jdk15ge-simple,!jdk15ge-add-open profiles, covers all 11 executable lines added by this PR with zero missed instructions or branches. In particular, the isLimitExceed() false branch is exercised. This is 100% local patch coverage, not whole-project coverage. Java Checkstyle and Spotless checks also pass with the same previously disclosed Windows Unix-resource-check exception. Remote CI and Codecov must rerun on the new commit; their previous success applies to the earlier commit only.

Checklist

Track successful admission per target and invocation attempt so downstream limit exceptions release acquired slots while local rejections leave other calls untouched.

Add filter-chain regression tests for synchronous and asynchronous failures, local rejection, invocation reuse, and target isolation.

Fixes apache#16455
@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.91%. Comparing base (dab47b7) to head (a20caa3).

Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #16456      +/-   ##
============================================
- Coverage     60.91%   60.91%   -0.01%     
- Complexity       15    11764   +11749     
============================================
  Files          1953     1953              
  Lines         89271    89280       +9     
  Branches      13473    13475       +2     
============================================
+ Hits          54383    54387       +4     
- Misses        29309    29315       +6     
+ Partials       5579     5578       -1     
Flag Coverage Δ
integration-tests-java21 32.13% <0.00%> (-0.01%) ⬇️
integration-tests-java8 32.20% <0.00%> (-0.01%) ⬇️
samples-tests-java21 32.16% <0.00%> (+<0.01%) ⬆️
samples-tests-java8 29.83% <0.00%> (-0.01%) ⬇️
unit-tests-java11 59.20% <100.00%> (+0.04%) ⬆️
unit-tests-java17 58.67% <100.00%> (+0.01%) ⬆️
unit-tests-java21 58.67% <100.00%> (+0.02%) ⬆️
unit-tests-java25 58.61% <100.00%> (+<0.01%) ⬆️
unit-tests-java8 59.18% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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] Concurrency limit filters leak active counts on downstream limit exceptions

2 participants