This is an automated email from the ASF dual-hosted git repository.
chrisdutz pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
The following commit(s) were added to refs/heads/develop by this push:
new 955486e0fb fix: Make the lease-time decision deterministic (purely
idle-time based, plus the force flag)
955486e0fb is described below
commit 955486e0fb42c858fc14aa9832e6533233fff300
Author: Christofer Dutz <[email protected]>
AuthorDate: Tue Jul 14 13:40:19 2026 +0200
fix: Make the lease-time decision deterministic (purely idle-time based,
plus the force flag)
---
.../java/utils/cache/ConnectionContainer.java | 71 +++++++---------------
.../cache/CachedPlcConnectionManagerTest.java | 11 ++--
2 files changed, 26 insertions(+), 56 deletions(-)
diff --git
a/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java
b/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java
index aff47dbb49..ff8c600ef9 100644
---
a/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java
+++
b/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java
@@ -74,7 +74,11 @@ class ConnectionContainer {
private final ConnectionStateTracker stateTracker;
private PlcConnection connection;
- private boolean requiresValidation;
+ // When true, the next lease MUST validate (ping) the connection
regardless of how recently it
+ // was used. Set when a lease is force-returned (e.g. by a max-lease
timeout) because the
+ // connection may be dead. In all other cases validation is decided purely
from idle time at
+ // lease() time, so it is deterministic and needs no scheduled task.
+ private boolean forceValidationOnNextLease;
// Tracks the last time a lease was successfully returned (i.e., the
connection was used).
// Used to determine if a ping validation is actually needed: if a
successful operation
// completed recently, the connection is known to be alive and pinging is
unnecessary.
@@ -91,7 +95,6 @@ class ConnectionContainer {
// Scheduled timeout tasks (must be canceled to prevent leaks)
private volatile ScheduledFuture<?> idleTimeoutTask;
- private volatile ScheduledFuture<?> requireValidationTimeoutTask;
private volatile ScheduledFuture<?> maxLeaseTimeoutTask;
/**
@@ -120,7 +123,7 @@ class ConnectionContainer {
this.idlePingThresholdMs = idlePingThresholdMs;
this.closeTimeoutMs = closeTimeoutMs;
this.queue = new LinkedList<>();
- this.requiresValidation = false;
+ this.forceValidationOnNextLease = false;
this.lastSuccessfulReturnTimeMs = 0;
this.currentLeaseId = 0;
this.lock = new ReentrantLock(true); // Fair lock to prevent thread
starvation
@@ -164,9 +167,14 @@ class ConnectionContainer {
// If validation fails, we need to close the existing
connection and get a new one.
// Skip validation if a successful operation completed
recently — the connection
// is known to be alive and pinging would just add unnecessary
blocking latency.
- else if (requiresValidation
- && (lastSuccessfulReturnTimeMs == 0
- || (System.currentTimeMillis() -
lastSuccessfulReturnTimeMs) >= idlePingThresholdMs)) {
+ //
+ // This decision is computed entirely here, at lease() time,
from the idle duration
+ // (plus the force flag). It does NOT depend on any scheduled
task having fired, so
+ // it is fully deterministic. A zero idlePingThreshold means
"always validate", which
+ // the (now - lastReturn) >= 0 comparison already yields.
+ else if (forceValidationOnNextLease
+ || lastSuccessfulReturnTimeMs == 0
+ || (System.currentTimeMillis() -
lastSuccessfulReturnTimeMs) >= idlePingThresholdMs) {
if (!validate()) {
LOGGER.debug("Connection failed validation, closing
and getting new connection");
closeBounded(connection);
@@ -266,13 +274,10 @@ class ConnectionContainer {
if(queue.isEmpty()) {
leasedConnection = null;
scheduleIdleTimeout();
- // Force validation if requested (e.g., from max lease
timeout) or if idlePingThresholdMs is 0
- if (forceValidation || idlePingThresholdMs == 0) {
- requiresValidation = true;
- } else {
- requiresValidation = false;
- scheduleRequireValidationTimeout();
- }
+ // A force-return means the connection may be dead: validate
on the next lease no
+ // matter how recently it was used. Otherwise leave it to the
idle-time check in
+ // lease() — no scheduled task needed.
+ forceValidationOnNextLease = forceValidation;
LOGGER.trace("Connection idle {}", connectionString);
return;
}
@@ -317,12 +322,9 @@ class ConnectionContainer {
// All queued entries had already timed out — go idle
leasedConnection = null;
scheduleIdleTimeout();
- if (forceValidation || idlePingThresholdMs == 0) {
- requiresValidation = true;
- } else {
- requiresValidation = false;
- scheduleRequireValidationTimeout();
- }
+ // See the queue-empty branch above: force validation only on a
force-return; otherwise
+ // the idle-time check in lease() decides deterministically.
+ forceValidationOnNextLease = forceValidation;
LOGGER.trace("Connection idle after all waiters timed out {}",
connectionString);
} finally {
lock.unlock();
@@ -469,27 +471,6 @@ class ConnectionContainer {
}
}
- private void scheduleRequireValidationTimeout() {
- // Cancel any existing max lease timeout
- cancelRequireValidationTimeout();
-
- try {
- requireValidationTimeoutTask = scheduler.schedule(() -> {
- // Mutate requiresValidation under the lock so the next
lease() reliably observes it.
- lock.lock();
- try {
- requiresValidation = true;
- } finally {
- lock.unlock();
- }
- }, idlePingThresholdMs, TimeUnit.MILLISECONDS);
- LOGGER.trace("Scheduled require validation timeout for {} in
{}ms", connectionString, idlePingThresholdMs);
- } catch (RejectedExecutionException e) {
- // Scheduler was already shut down (manager shutting down)
- LOGGER.debug("Cannot schedule require validation timeout,
scheduler shut down: {}", connectionString);
- }
- }
-
/**
* Cancel the idle timeout task if running.
*/
@@ -512,16 +493,6 @@ class ConnectionContainer {
}
}
- /**
- * Cancel the require-validation timeout task if running.
- */
- private void cancelRequireValidationTimeout() {
- if (requireValidationTimeoutTask != null) {
- requireValidationTimeoutTask.cancel(false);
- requireValidationTimeoutTask = null;
- LOGGER.trace("Cancelled require validation timeout for {}",
connectionString);
- }
- }
/**
* Close this connection container and the underlying connection.
diff --git
a/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManagerTest.java
b/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManagerTest.java
index 25827d83bc..9efb7e31a9 100644
---
a/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManagerTest.java
+++
b/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManagerTest.java
@@ -593,12 +593,11 @@ class CachedPlcConnectionManagerTest {
PlcConnection connection1 =
manager.getConnection("test:tcp://localhost");
connection1.close();
- // Wait for connection to become idle beyond threshold.
- // Use a generous margin over the 500ms threshold: the connection is
only marked
- // "requires validation" by a task on the (single-thread) scheduler
that fires at the
- // threshold, and on a loaded CI runner that task can be delayed. A
small margin
- // (e.g. 100ms) makes this test flaky; 1500ms gives the scheduler
ample room.
- Thread.sleep(1500);
+ // Wait for the connection to become idle beyond the 500ms threshold.
The ping decision is
+ // made deterministically from the idle duration at lease() time (no
scheduled task
+ // involved), so any wall-clock margin over the threshold suffices;
800ms is comfortably
+ // clear of scheduling jitter on a loaded CI runner.
+ Thread.sleep(800);
// Reset ping mock to track subsequent calls
reset(mockConnection);
