Skip to content

Fix race condition in ChannelCoordinator startup: wait for receiver readiness before reading localMember - #1062

Open
ABin-Huang wants to merge 4 commits into
apache:mainfrom
ABin-Huang:fix/channel-coordinator-startup-race-v2
Open

Fix race condition in ChannelCoordinator startup: wait for receiver readiness before reading localMember#1062
ABin-Huang wants to merge 4 commits into
apache:mainfrom
ABin-Huang:fix/channel-coordinator-startup-race-v2

Conversation

@ABin-Huang

Copy link
Copy Markdown

Problem

ChannelCoordinator.internalStart() calls clusterReceiver.start() and then immediately calls getChannel().getLocalMember(false) without waiting for the receiver's background thread to enter the listen loop. This creates a race window where:

  1. The NioReceiver listener thread may not have executed setListen(true) yet
  2. The membershipService may not have initialized localMember
  3. The local member's host/port properties may be unset or default values

This can cause intermittent cluster communication failures where nodes cannot connect to each other because the advertised host/port is incorrect.

The original code had a // synchronize, big time FIXME comment at this location.

Fix

Added a readiness signaling mechanism based on CountDownLatch:

  1. ChannelReceiver interface: Added waitForReady(long timeout, TimeUnit unit) default method that returns true immediately (backward compatible). Added DEFAULT_READY_TIMEOUT_MS constant.

  2. NioReceiver: Added volatile CountDownLatch readyLatch. A fresh latch with count=1 is created in start() before launching the listener thread. The latch is counted down in listen() after setListen(true). waitForReady() is overridden to await the latch. Latch is reset to count=0 in stopListening() to support restarts.

  3. ChannelCoordinator: After clusterReceiver.start(), calls waitForReady(5000ms) before reading getLocalMember(). Throws ChannelException on timeout or interruption. Removed the FIXME comment.

Testing

Added TestChannelCoordinatorStartupRace with 5 test cases:

  1. testWaitForReadyDefaultMethodReturnsImmediately - Verifies backward compatibility of the default method
  2. testNioReceiverReadyLatchContract - Verifies CountDownLatch lifecycle via reflection
  3. testChannelCoordinatorWaitsForReceiverBeforeLocalMember - Verifies exact call order: start() → waitForReady() → getLocalMember() → setLocalMemberProperties()
  4. testChannelCoordinatorThrowsWhenReceiverNotReady - Verifies ChannelException on timeout and that getLocalMember is NOT called
  5. testChannelCoordinatorHandlesInterruptedException - Verifies interrupt status is restored and ChannelException is thrown

Files Changed

  • java/org/apache/catalina/tribes/ChannelReceiver.java - Add waitForReady() default method
  • java/org/apache/catalina/tribes/transport/nio/NioReceiver.java - Add CountDownLatch readiness signaling
  • java/org/apache/catalina/tribes/group/ChannelCoordinator.java - Wait for receiver readiness before reading localMember
  • java/org/apache/catalina/tribes/group/LocalStrings.properties - Add new i18n messages
  • test/org/apache/catalina/tribes/group/TestChannelCoordinatorStartupRace.java - Add unit tests

Note: This PR supersedes #1061 and #1060. The branch is based on the latest main (fork synced to 719edeaa).

…eadiness

Add waitForReady() default method to ChannelReceiver interface and
CountDownLatch readiness signaling to NioReceiver, based on latest main.
…dinator

Call clusterReceiver.waitForReady() after start() and before getLocalMember(),
eliminating the race window where the listener thread may not have entered
the select loop yet. Throw ChannelException on timeout or interruption.
Also add new i18n messages for timeout/interruption scenarios.
Add TestChannelCoordinatorStartupRace with 5 test cases covering:
- Default method backward compatibility
- CountDownLatch lifecycle contract
- Exact call order verification
- Timeout path
- Interrupt handling
Fix regressions introduced in the v2 rebuild: ChannelReceiver.java must keep
MAX_UDP_SIZE (referenced by NioReceiver.listen()) and its original JavaDoc;
LocalStrings.properties must keep all existing keys and the license header.
Only the waitForReady() default method, DEFAULT_READY_TIMEOUT_MS and the two
new i18n messages are added on top of the current main content.
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