Repository navigation
fix(plc4j/spi): RequestThrottle silently drops a limit reduction while requests are in flight - #2734
Conversation
|
Hi @alxkm , Thanks for opening this Pull Request. Per our updated contribution guidelines, we ask that contributors who plan to introduce substantial changes or new capabilities—such as work touching our Serial Peripheral Interface (SPI)—first introduce themselves on the dev mailing list (dev@plc4x.apache.org). Could you please post a brief introduction on the mailing list covering: A quick introduction of yourself and your context. Your goals and intent for this PR, specifically what you are looking to achieve with these changes to the SPI. (Your use case and machines etc.) Open-source maintainability relies heavily on context and architectural alignment before reviewing code diffs. Introducing yourself on the list gives the broader community a chance to understand the use case, offer early architectural feedback, and coordinate efforts effectively. Once you've posted to the list, please link the thread or drop a note here so we can align on the proposed changes before proceeding with the code review. Thanks! This helps us to assure maintainability and is a way against AI Slop protection with Pull requests just opening but no intent on further contributions. |
|
Hi @ottlukas Thanks - makes sense. Posted an introduction and the context behind this PR Short version: no hardware and no production use case behind it, I came to |
…be applied at once
adjustMaxConcurrentRequests() took permits away with
int toDrain = Math.min(-difference, semaphore.availablePermits());
and then updated maxConcurrentRequests unconditionally. A permit that is in
flight cannot be taken back, so whenever more requests were running than the new
limit allows, the clamped-away part of the reduction was silently discarded
while the field claimed it had been applied. As the running requests returned
their permits the semaphore climbed back to the old count and stayed there, so
the throttle permanently allowed more concurrent requests than it was configured
for - the one thing the class exists to prevent. Repeated adjustments compound
it: shrinking under load and raising the limit again leaves the pool over budget.
This is not limited to the case where no permit is free. With 3 permits, 2 of
them in flight, reducing to 1 takes the single free permit and drops the second.
Remember the part that cannot be applied immediately as a permit debt and settle
it out of the permits the running requests return, so the reduction takes effect
as soon as it can rather than being lost. Reducing no longer blocks either: the
old acquireUninterruptibly() could wait, under the instance monitor, for permits
that only the in-flight requests could hand back, because availablePermits() may
drop between the check and the call.
getInFlightRequests() derived its result from the maximum and the free permits,
which could go negative - already before this change, on the growth path, where
release(difference) ran before the field was updated, so a request started by
the drain in between saw a negative count. Track the permits handed out in a
counter instead, which cannot go negative and does not depend on an adjustment
being atomic with the read.
An adjustment that could not be applied in full now says so in the log; without
that there is nothing to go on when concurrency does not drop as expected.
The 15 existing tests are unchanged and keep passing. 12 are added. Four of them
read the permit debt through a package-private accessor and therefore only
compile against this version; drop those four together with their helper and the
remaining 8 compile against develop, where all 8 fail.
Signed-off-by: alxkm <alexanderklmn@gmail.com>
a4dbce1 to
6b1c361
Compare
…t-limit-reduction
…t-limit-reduction
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Permit release can race with limit reduction, temporarily allowing concurrency above the configured maximum.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes permit-limit reductions under load by tracking deferred permit debt and actual in-flight requests.
Changes:
- Retires returned permits against outstanding reduction debt.
- Cancels debt before increasing available permits.
- Adds regression and permit-accounting tests.
| File | Description |
|---|---|
RequestThrottle.java |
Implements permit debt and direct in-flight tracking. |
RequestThrottleTest.java |
Tests reductions, increases, and accounting invariants. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…one lock A permit returned while adjustMaxConcurrentRequests() was applying a reduction could be counted twice. The completing request read a debt of zero and released its permit, while the reduction failed to acquire that same permit and recorded it as debt instead, so the throttle offered free permits although the reduced limit was already taken by the requests still running. The debt check and the release now run under the same lock as the reduction's try-acquire, so a returned permit either goes back to the pool or retires part of the reduction, never both. drain() runs outside the lock, so no request is started while it is held.
…t-limit-reduction
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
maxConcurrentRequestsis written underpermitLockinadjustMaxConcurrentRequests, but read… · Newready.await(...)is ignored. If it times out, the test may proceed with… · Newunsettled > 0, butunsettledcan… · New
1 resolved since last review
🧠 Review effort: Lite
- Word the unsettled-debt log message so it also fits a raise that only cancels part of an earlier reduction. - Fail fast when the race test's threads do not all get ready, instead of running the attempt with the threads not aligned. - Run the race test 500 times instead of 2000; on the unfixed code it fails within the first couple of hundred attempts.
…t-limit-reduction
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
adjustMaxConcurrentRequestsis no longersynchronized, butmaxConcurrentRequestsis still… · NewadjustMaxConcurrentRequestsis no longersynchronized, butmaxConcurrentRequestsis still… · NewawaitGospin-waits indefinitely and ignores interruption. If the test fails beforego.set(true)… · New
5 resolved since last review
ready.await(...)is ignored. If it times out, the test may proceed with…maxConcurrentRequestsis written underpermitLockinadjustMaxConcurrentRequests, but read…unsettled > 0, butunsettledcan…
🧠 Review effort: Lite
… fails If an attempt failed before every thread was let go, the threads still in the spin loop ignored the interrupt from shutdownNow() and kept spinning for the rest of the test JVM. They now give up when interrupted. The test also gets an overall timeout, so a slow or contended machine cannot stretch it out without bound.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
adjustMaxConcurrentRequestsno longer synchronizes onthis(it now usespermitLock) but… · New
5 resolved since last review
awaitGospin-waits indefinitely and ignores interruption. If the test fails beforego.set(true)…adjustMaxConcurrentRequestsis no longersynchronized, butmaxConcurrentRequestsis still…adjustMaxConcurrentRequestsis no longersynchronized, butmaxConcurrentRequestsis still…
🧠 Review effort: Lite
… time While waiting to be let go, the race test's threads now spin briefly and then park between checks instead of spinning the whole time. Besides not holding on to a core, this lets go of the threads closely enough to hit the race within the first few attempts on the unfixed code, so 100 attempts are enough. The attempts are also bounded by time, so a slow machine runs fewer of them instead of running into the timeout.
…t-limit-reduction
RequestThrottle.adjustMaxConcurrentRequests()loses the part of a reduction it cannot apply while requests are in flight, leaving the throttle permanently above its configured limit. This change defers that part as a permit debt that is settled as the running requests complete.Problem
adjustMaxConcurrentRequests()took permits away withand then updated
maxConcurrentRequestsunconditionally. A permit that is in flight cannot be taken back, so whenever more requests were running than the new limit allows, the clamped-away part of the reduction was silently discarded while the field claimed it had been applied. As the running requests returned their permits, the semaphore climbed back to the old count and stayed there: the throttle permanently allowed more concurrent requests than it was configured for, which is the one thing the class exists to prevent. Repeated adjustments compound it - shrinking under load and raising the limit again leaves the pool over budget for good.This is not limited to the case where no permit is free. With 3 permits and 2 in flight, reducing to 1 takes the single free permit and drops the second;
getInFlightRequests()then reports 1 while 2 requests are running, and -1 once they finish.Fix
The part of a reduction that cannot be applied immediately is remembered as a permit debt and settled out of the permits the running requests return, so the reduction takes effect as soon as it can instead of being lost. Raising the limit cancels outstanding debt before handing out new permits. The adjustment log line reports how much of a reduction is still outstanding.
Reducing no longer blocks. The old
acquireUninterruptibly()could wait, under the instance monitor, for permits that only the in-flight requests could hand back, becauseavailablePermits()may drop between the check and the call. Free permits are now taken with non-blockingtryAcquire(), and whatever is left becomes debt.getInFlightRequests()now counts the permits handed out directly instead of deriving the number from the ceiling and the free permits. The derived value could read negative not only after a lost reduction but also while the limit was being raised, because the permits were released before the ceiling was updated. While a debt is outstanding the counter may temporarily exceedgetMaxConcurrentRequests(); that is the number of requests actually running, and it settles as they complete.Tests
The 15 existing tests are unchanged and keep passing. The 8 added tests all fail without this change. A package-private
getPermitDebt()exposes the outstanding debt to the tests.Reachability
RequestThrottleis afinalfield onConnectionBase, so it outlives any number ofconnect()calls, andconnect()has no re-entry guard: an application-driven reconnect re-runs S7'sSetupCommunicationand adjusts a throttle that is already serving traffic. No driver triggers this on its own today, so this is a latent defect rather than an observed production failure.Alternative considered
A larger change would drop the semaphore from the
execute()path in favour of anAtomicIntegerin-flight counter checked against avolatilemaximum, which would make a reduction a plain assignment. That touches the publicacquire()/release()pair - unused elsewhere in the tree, but public API - so it is left out of this change.