Skip to content

[fix][client] Fix waiting lookup requests not dispatched on timeout in ClientCnx#26143

Open
geniusjoe wants to merge 1 commit into
apache:masterfrom
geniusjoe:dev/lookup-timeout
Open

[fix][client] Fix waiting lookup requests not dispatched on timeout in ClientCnx#26143
geniusjoe wants to merge 1 commit into
apache:masterfrom
geniusjoe:dev/lookup-timeout

Conversation

@geniusjoe

Copy link
Copy Markdown
Contributor

main pr #25038

Motivation

PR #25038 fixed the semaphore leak issue by adding a TimeoutException check in the whenComplete callback of newLookup(). However, it did not fully address the waiting queue starvation problem.

When a lookup request times out in checkRequestTimeout(), the code only calls pendingRequests.remove() and relies on the whenComplete callback to release the semaphore. While the semaphore is correctly released, getAndRemovePendingLookupRequest() — which is responsible for polling and dispatching the next waiting request from waitingLookupRequests — is never invoked on the timeout path.

This means: if all concurrent lookup slots time out simultaneously (e.g., during a broker GC pause or network partition), the waiting requests remain stuck indefinitely even though semaphore permits become available. They will never be dispatched until a new non-timeout lookup response arrives.

Modifications

Optimized the timeout handling path in ClientCnx.java:

  1. checkRequestTimeout(): For Lookup-type requests, use getAndRemovePendingLookupRequest() instead of plain pendingRequests.remove(). This ensures the next waiting request in waitingLookupRequests is polled and dispatched upon timeout, rather than just releasing the semaphore.

  2. newLookup() whenComplete callback: Removed the TimeoutException check since getAndRemovePendingLookupRequest() already handles semaphore release and queue driving. Keeping it would cause a double-release.

Verifying this change

This change added tests and can be verified as follows:

  • testLookupTimeoutReleasesSemaphore — verifies that the semaphore is properly released after a lookup timeout.
  • testLookupTimeoutDrivesWaitingQueue — verifies that when a concurrent lookup times out, the next waiting request in the queue is dispatched.
  • testMultipleLookupTimeoutsNoSemaphoreLeak — verifies that with multiple concurrent and queued lookups all timing out, every request eventually completes and no semaphore permits are leaked.

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Local workflow:

geniusjoe#1

@geniusjoe

Copy link
Copy Markdown
Contributor Author

@congbobo184
Hi! Since you're familiar with this area from #25038, would you mind taking a look at this PR when you have a chance? It optimizes the timeout path to also drive the waiting lookup queue. Thanks in advance

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.

1 participant