Conversation
| } catch (RejectedExecutionException e) { | ||
| log.debug("Skip pong timeout for {}, timer stopped or queue full", |
There was a problem hiding this comment.
[SHOULD] Preserve timeout handling when the queue is full
This catch block only logs the rejection, but waitForPong has already been set to true and the Ping has been sent. If no Pong arrives, the node has no timeout task to trigger retries or transition it to DEAD. The rejected task is not rescheduled when queue capacity becomes available.
External UDP traffic can fill the shared queue and cause legitimate nodes discovered during that period to hit this branch. Compared with the previous behavior, these nodes lose automatic retries when a Ping or Pong is dropped.
Please add bounded recovery or retry handling for rejected submissions, along with a regression test that fills the queue, discovers a new node, withholds its Pong, and then frees queue capacity. Verify that the node resumes retries or reaches an explicit failure state.
| }, KadService.getPingTimeout(), TimeUnit.MILLISECONDS); | ||
| }, KadService.getPingTimeout(), TimeUnit.MILLISECONDS); | ||
| } catch (RejectedExecutionException e) { | ||
| log.debug("Skip pong timeout for {}, timer stopped or queue full", |
There was a problem hiding this comment.
[SHOULD] Could we distinguish queue-capacity rejection from shutdown rejection and expose the former through a rate-limited warning or a rejection counter?
At this point, the Ping has already been sent, but its timeout task was not scheduled. This can prevent timeout-driven retries or state transitions for affected nodes, so queue saturation is operationally relevant. The current debug-only message is invisible at INFO level and does not identify which condition occurred.
Shutdown rejection can remain at DEBUG. For queue saturation, please use a globally rate-limited summary or an exposed counter rather than a warning for every rejected task.
| @Override | ||
| public synchronized ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) { | ||
| // Keep the capacity check and enqueue atomic across discovery and retry threads. |
There was a problem hiding this comment.
[NIT] The capacity check only covers schedule(Runnable, ...). Other entry points, including schedule(Callable, ...), submit(Runnable, result), and periodic scheduling, bypass it.
The current NodeHandler.sendPing() call is covered, so this does not invalidate the limit for the existing production path. However, getPongTimer() exposes the full scheduling API, making it easy for future callers to bypass the limit unintentionally.
Could we keep the executor private and expose a dedicated schedulePongTimeout(...) method instead? This would make the supported submission path explicit without requiring a general-purpose bounded scheduler.
| kadService.getPongTimer().schedule(() -> { | ||
| try { |
There was a problem hiding this comment.
[SHOULD] Could we retain the ScheduledFuture and cancel the corresponding timeout when a Pong is accepted? Currently, completed Ping/Pong exchanges leave timeout tasks queued until expiry, consuming capacity under the new limit.
Please also review timeout cleanup when trimTable() removes a handler. Its pending tasks still reference it and may continue retrying. This should include resolving any in-flight eviction challenge, since removing a handler from the map does not necessarily end its role in the routing state machine.
Cancelled tasks should be removed promptly, for example with setRemoveOnCancelPolicy(true). Cleanup should also account for races with an executing timeout, so stale callbacks cannot schedule further retries after the handler has been retired.
|
|
||
| private static final int MAX_NODES = 2000; | ||
| private static final int NODES_TRIM_THRESHOLD = 3000; | ||
| static final int MAX_PENDING_PONG_TASKS = 2000; |
There was a problem hiding this comment.
[Question] Just wondering why we chose 2,000 here instead of another value.
What does this PR do?
MAX_PENDING_PONG_TASKS = 2000. Serialize the capacity check and enqueue operation in theschedule(Runnable, ...)entry point used by discovery timeouts, so concurrent receive and retry threads cannot exceed the queue limit. Capacity becomes available again as tasks leave the queue.RejectedExecutionExceptioninNodeHandler.sendPing(). When the queue is full or the executor stops, skip the timeout task and log at debug level instead of propagating the exception into the UDP channel's error handler.Why are these changes required?
UDP discovery creates a pong timeout task for each newly observed endpoint. Removing handlers from the node map does not cancel these tasks, so the scheduler can retain handlers beyond the map's trimming limits. This PR provides a focused mitigation for that timeout backlog by limiting queued tasks and handling overflow safely.
This PR has been tested by:
Added 3 regression tests covering queue overflow, reuse of capacity after execution, concurrent submissions, and continued UDP processing under saturation.
The decoder-path test processes 4,000 valid datagrams from distinct source ports on the same IP. The queue stays at 2,000 tasks and the channel remains active. It also confirms the remaining scope: all 4,000 initial pings are emitted and the existing map trimming leaves 999 handlers. Outbound delivery is captured in-process; this is not a resource-exhaustion load test.
All 27 discovery tests passed locally on
7f644ed:Follow up
NodeHandlerstate before proving they can receive and respond to a challenge.Extra details
The limit applies to queued pong timeout tasks; an executing callback is outside that count. It is enforced at the submission entry point used by discovery, rather than by replacing the JDK executor's internal queue.