Skip to content

fix(udp): limit queued pong timeout tasks - #148

Open
317787106 wants to merge 1 commit into
tronprotocol:release-v2.3.0from
317787106:fix/fix_unbound_ponger
Open

317787106 wants to merge 1 commit into
tronprotocol:release-v2.3.0from
317787106:fix/fix_unbound_ponger

Conversation

@317787106

@317787106 317787106 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

  • Add the fixed constant MAX_PENDING_PONG_TASKS = 2000. Serialize the capacity check and enqueue operation in the schedule(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.
  • Catch RejectedExecutionException in NodeHandler.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.
  • Keep the change limited to timeout scheduling and rejection handling, with no new configuration options. Existing accessor methods are replaced with equivalent Lombok-generated accessors.

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:

./gradlew test --tests 'org.tron.p2p.discover.*' --console=plain

Follow up

  • Return-path verification: new endpoints still create NodeHandler state before proving they can receive and respond to a challenge.
  • Admission and rate limits: there is no new per-IP limit, endpoint creation rate limit, or independent quota for unverified handlers. The existing node-map trimming behavior remains in place.
  • Initial processing and outbound traffic: the initial ping is sent before timeout scheduling. Rejecting a timeout task does not prevent packet parsing, handler creation, map trimming, or that outbound ping.
  • Task lifecycle cleanup: node-map trimming and receiving a PONG still do not cancel and remove already queued tasks. Accepted tasks can continue retaining removed handlers and performing retries under the existing retry rules, subject to the queue limit.
  • Availability under saturation: unverified endpoints can occupy the shared timeout capacity, causing legitimate probes' timeout/retry scheduling to be rejected as well. The cap bounds the queued timeout backlog but does not fully address UDP discovery resource exhaustion or ensure fair admission.

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.

@317787106 317787106 changed the title fix(net): add MAX_PENDING_PONG_TASKS to avoid ddos task fix(discovery): limit queued pong timeout tasks Sep 10, 2026
Comment on lines +175 to +176
} catch (RejectedExecutionException e) {
log.debug("Skip pong timeout for {}, timer stopped or queue full",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@317787106 317787106 changed the title fix(discovery): limit queued pong timeout tasks fix(udp): limit queued pong timeout tasks Sep 11, 2026
}, KadService.getPingTimeout(), TimeUnit.MILLISECONDS);
}, KadService.getPingTimeout(), TimeUnit.MILLISECONDS);
} catch (RejectedExecutionException e) {
log.debug("Skip pong timeout for {}, timer stopped or queue full",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +67 to +69
@Override
public synchronized ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
// Keep the capacity check and enqueue atomic across discovery and retry threads.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +165 to +166
kadService.getPongTimer().schedule(() -> {
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Just wondering why we chose 2,000 here instead of another value.

@317787106
317787106 changed the base branch from main to release-v2.3.0 September 15, 2026 04:00
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.

2 participants