[jira] [Commented] (CAMEL-25300) Throttle EIP - the clean of the total requests throttler can hand out a second set of permits, so more exchanges than allowed pass in a time period

["Claus Ibsen (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CAMEL-25300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123448#comment-18123448 ] 

Claus Ibsen commented on CAMEL-25300:
-------------------------------------

Merged to main via https://github.com/apache/camel/pull/27341 (commit 30791176edb5), fix version 4.23.0.

_Claude Code on behalf of davsclaus_

> Throttle EIP - the clean of the total requests throttler can hand out a second set of permits, so more exchanges than allowed pass in a time period
> ---------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: CAMEL-25300
>                 URL: https://issues.apache.org/jira/browse/CAMEL-25300
>             Project: Camel
>          Issue Type: Bug
>          Components: camel-core
>            Reporter: shashank
>            Assignee: shashank
>            Priority: Minor
>             Fix For: 4.23.0
>
>
> {{TotalRequestsThrottler}} keeps a {{ThrottlingState}} (a {{DelayQueue}} of permits) per correlation key. Every returned permit schedules {{clean()}} 10 periods later ({{cleanPeriodMillis = timePeriodMillis * 10}}, computed once in the constructor), which does {{states.remove(key)}}.
> * An exchange that looked up the state just before the clean runs still polls a permit from the removed state, while the next exchanges create a new state with all {{maximumRequests}} permits available at once. So in that period more exchanges than allowed pass (with 2 per period: 3).
> * The permit of that exchange is returned to the removed state, which schedules the removed state's clean again. 10 periods later that clean does {{states.remove(key)}}, which removes whatever state is mapped then, also the new one while it is in use: its delayed permits are dropped, the next exchange gets a fresh state with all permits, and the clean of the dropped state repeats this. Under continuous traffic with concurrent senders the throttler can keep resetting every 10 periods.
> * {{setTimePeriodMillis}} (JMX) does not change the clean period. After increasing the period to more than 10 times its initial value, the clean removes states whose permits are still delayed, and the next exchanges get new permits at once.
> CAMEL-24926 fixed the same pattern in {{ConcurrentRequestsThrottler}}: its clean only removes its own state when no permit is out, and an exchange that took a permit from a removed state takes it from the current state instead.
> h3. Reproduction
> Test {{TotalRequestsThrottlerCleanTest}} (camel-core, the same technique as {{ConcurrentRequestsThrottlerCleanTest}} from CAMEL-24926: an executor that keeps the scheduled clean tasks, and the {{maximumRequests}} expression, which the throttler evaluates after it looked up the state, runs them for a given exchange). Throttle 2 per hour, {{rejectExecution(true)}}:
> * {{testCleanAfterExchangeLookedUpState}}: "first", then "race" (the clean runs after it looked up the state, as if 10 periods had passed), then "second" must pass and the next exchange must be rejected. On main it passes too (3 exchanges in the period after the clean):
> {noformat}
> TotalRequestsThrottlerCleanTest.testCleanAfterExchangeLookedUpState:64 Expected org.apache.camel.CamelExecutionException to be thrown, but nothing was thrown.
> {noformat}
> * {{testCleanFollowsTimePeriod}}: after {{setTimePeriodMillis(2 hours)}} the clean is scheduled in 20 hours. On main:
> {noformat}
> TotalRequestsThrottlerCleanTest.testCleanFollowsTimePeriod:94 expected: <[72000000]> but was: <[36000000]>
> {noformat}
> * {{testCleanRemovesUnusedState}} (control): the clean still removes an unused state (passes on main).
> The defect was found with a TLA+ model of the throttler states, the clean tasks, exchanges (lookup, poll, enqueue as separate steps) and time periods: "at most maximumRequests permits are handed out per period" is violated in 12 steps (an exchange looks up the state, the clean removes it, the exchange takes a permit, another exchange creates a new state and takes a permit), and "the clean of a replaced state never removes the current state" in 13 steps. With the fix below both hold (2-3 exchanges, 1-2 permits, 2-4 periods).
> h3. Proposed fix
> As in CAMEL-24926:
> * {{clean()}}: {{states.computeIfPresent(key, (k, s) -> s == this && markRemovedIfUnused() ? null : s)}}; a state is unused when all its permits are back in the queue (checked and marked {{removed}} under a lock of its own).
> * Unlike CAMEL-24926, that lock is not the state's lock: in this mode a decrease of the maximum requests (a dynamic {{maximumRequests}} expression, such as a header) takes permits with {{DelayQueue.take()}} while it holds the state's lock, and waits when none is available. An exchange that took a permit must be able to check the flag and return its permit meanwhile, otherwise the decrease and every exchange of the key wait forever. (The concurrent requests throttler decreases with {{Semaphore.reducePermits}}, which does not wait.)
> * An exchange that polled (or took) a permit from a state that is marked removed puts the permit back and takes one from the current state.
> * The clean is scheduled with the current time period ({{10 x getTimePeriodMillis()}}, saturated). This belongs to the same fix: "unused" counts the permits in the queue, delayed or not, so a clean must not run while a permit is still delayed; with the period fixed at the initial value it could, after the period was increased.
> {{TotalRequestsThrottlerRateDecreaseTest}} (regression test for the lock above): {{throttle(header("max")).timePeriodMillis(2000)}}; two exchanges take both permits, two more wait in {{take()}} (the test waits until their threads are parked), then an exchange with {{max=1}} decreases the maximum and waits for a permit to discard. All five must get through. It passes on main; with the first version of the fix (the flag under the state's lock) it hung: the thread dump shows the two exchanges parked in {{isRemoved()}} on the state's lock and the decreasing one in {{DelayQueue.take()}} inside {{calculateAndSetMaxRequestsPerPeriod}}.
> With the fix the new tests and the throttle tests ({{org/apache/camel/processor/throttle/**}}, {{Throttl*Test}}: 60 tests) pass.
> Severity: the lookup race needs an exchange at the instant the clean runs (after 10 idle periods); once it happened, the reset can repeat under continuous traffic; the JMX case is deterministic. Minor.
> Affected: 4.14.x, 4.18.x and main (same code).
> Duplicate check (2026-10-03): JIRA text "TotalRequestsThrottler" (CAMEL-24928 asyncDelayed NPE, CAMEL-24227 volatile JMX fields), summary "throttle"/"throttler" since 2023 (CAMEL-24926, CAMEL-20357, CAMEL-20267, ...: none about the clean of the total requests mode). GitHub pull requests "TotalRequestsThrottler", "throttler clean": #26775 (CAMEL-24928), #24985, #24713; the open #26871 touches {{ConcurrentRequestsThrottler}} interrupt handling only.
> _Filed with Claude Code on behalf of allthingssecurity._



--
This message was sent by Atlassian Jira
(v8.20.10#820010)