allthingssecurity opened a new pull request, #26945: URL: https://github.com/apache/camel/pull/26945
# Description [CAMEL-25062](https://issues.apache.org/jira/browse/CAMEL-25062) Follow-up to CAMEL-24545 (#25842). That fix stopped `ZooKeeperClusterView` from firing a leadership event while it stops, which removed one trigger. The lock inversion it was about is in `ClusteredRoutePolicy` and is still there for every cluster service: - A cluster view calls its leadership listeners while it holds its read lock (`AbstractCamelClusterView.doWithListener`). The listener of `ClusteredRoutePolicy` takes the policy lock and starts or stops the routes on that same thread, and `startRoute` needs the CamelContext route lock. - `releaseClusterView` (from `onRemove` of the last route, and from `doShutdown`) takes the policy lock and then calls `removeEventListener`, which needs the write lock of the view. `CamelContext.removeRoute` holds the CamelContext route lock while it calls `onRemove`. `ClusteredRoutePolicyFactory` creates one policy per route, so a leadership event starts the routes one after the other inside the dispatch. If the CamelContext is stopped, or a route is removed, while a route is still being started (a consumer that takes a while to connect, which a rolling restart easily hits), the threads deadlock: - CamelContext stop: the stop holds the lock of a policy further down the list and waits for the write lock of the view; the dispatch waits for that policy lock. - removeRoute: the operator holds the CamelContext route lock and waits for the policy lock (or, if the release took no policy lock, for the write lock of the view); the dispatch holds the read lock of the view and waits in `startRoute` for the CamelContext route lock. With `FileLockClusterService` the stuck dispatch thread is the leadership thread of the service, so the node stops writing its heartbeat but keeps the file lock, and no other node can become the leader until the JVM is killed. This change, in `ClusteredRoutePolicy`: - The leadership listener hands the change to the policy's own thread, which starts or stops the routes under the policy lock. The view lock is never held while the CamelContext route lock is awaited, and a slow route start no longer blocks the leadership thread of the cluster service. Making the release lock-free alone is not enough, see the tests below. - Threading: the thread comes from a pool created through the `ExecutorServiceManager` (so it is named `ClusteredRoutePolicy` and managed like the other Camel pools) with no core thread, at most one thread, a keep-alive of one second and the `Abort` rejection policy. One thread at most keeps the changes of a policy in order, and the thread exits when the leadership does not change, so `ClusteredRoutePolicyFactory` (a policy per route) does not keep a thread per route, and a removed route leaves no thread behind. When the leadership changes, each policy uses a thread for about a second; the route starts are serialized by the CamelContext anyway. I did not use one shared thread per CamelContext: it would need an owner with its own lifecycle, and a slow route start would delay the changes of every other policy and namespace. - The policy thread reads the leadership when it applies a change, so the last change wins, and at most one change is queued: a change that is still queued covers the next ones. A change is never applied on the notifying thread. Once the policy has been shut down the pool rejects it, which is logged at DEBUG and ignored, and an exception while applying a change is logged at WARN. - `retainClusterView` and `releaseClusterView` take no policy lock. The view is kept in an `AtomicReference` and swapped atomically. The release only resets the leader flag, as the routes of the policy are removed or shut down already, so there is nothing left to stop. Retain and release are serialized by a separate small lock, so a route added and a route removed at the same time on a shared policy cannot release the view that has just been retained. Only the threads that add or remove routes (and the shutdown) take it, and they never wait for the policy lock or the policy thread while they hold it. - `retainClusterView` reads the current leadership when it retains the view. The event that `addEventListener` fires is now applied asynchronously, and `onInit` uses the flag to let the route controller start a route added while this node is the leader. - `onCamelContextStarted` checks the deferred start flag (`startManagedRoutesEarly`) under the policy lock, as the policy thread may now apply a leadership change while the CamelContext is starting. - A change that reaches the policy thread while the CamelContext is stopping is ignored, whether the leadership is taken or lost, in line with CAMEL-24545. Taking it would start routes during the stop. For a loss, the CamelContext is already stopping all the routes, starting with their consumers, and stopping a route from the policy thread would run a second shutdown of that route through the same `ShutdownStrategy` while the CamelContext uses it. - The scheduler for `initialDelay` is created only when a delay is set, and is shut down once the delay has elapsed. - The route sets are concurrent sets, as the policy thread and the thread adding or removing routes use them at the same time. Behaviour change (upgrade guide 4.23): the routes are started and stopped shortly after the leadership event, on the policy thread, instead of inside the event. Most cluster services fire the event from their own threads, but some fire it on the calling thread, for example when a view starts or a listener is added (`AbstractCamelClusterView.addEventListener`, `JGroupsRaftClusterView` and `FileLockClusterView` on start). Code that fires the event itself, as the tests in camel-core do, can no longer expect the routes to be started when the event returns. `isLeader()` (JMX `Leader`) reflects the view right away, and the routes follow. The existing `ClusteredRoutePolicyTest`, `ClusteredRoutePolicyFactoryTest`, `ClusteredRoutePolicyLeaderChangeTest` and `JGroupsRaftClusteredRoutePolicyTest` now wait for the route status (Awaitility) where they asserted it right after the leadership change. Not changed: `ZooKeeperClusterView` keeps the CAMEL-24545 guard, and the views still do not fire a leadership event when they are stopped. Once this is merged, views could notify listeners that are still registered when a view is force-stopped (JMX `stopView`); that is left for a separate issue. Also not changed: `onInit` still calls `startManagedRoutes()` without the policy lock, as it runs under the route model lock, and a shared policy is still inert after a CamelContext restart (same as before). An alternative would be to iterate over a snapshot of the listeners in `AbstractCamelClusterView.doWithListener`. That changes every cluster view and camel-master, so I kept the change in the policy. Tests: new `ClusteredRoutePolicyReleaseDeadlockTest` in camel-core. It does not extend `ContextTestSupport`, so a regression fails the test instead of hanging the tear-down. Three routes use `ClusteredRoutePolicyFactory`: "slow", whose consumer start waits on a latch, then "other" and "late". A gate listener, registered between the listeners of "slow" and "other", holds the leadership dispatch until the view is being released. No sleeps; `assertTimeoutPreemptively` turns a hang into a failure. - `testCamelContextStopDuringLeadershipChange`: the leadership is taken, the start of "slow" is held, the CamelContext is stopped, and once the stopping thread is waiting the start is released. The stop and the dispatch must finish, and the policies must release the view. - `testRemoveRouteDuringLeadershipChange`: the same with `stopRoute("other")` and `removeRoute("other")`. The removal must return, and "slow" and "late" must end up started. The policy of "late" is still registered when the dispatch reaches it, so it has to start its route while `removeRoute` holds the CamelContext route lock. - `testLeadershipChangesStartAndStopRoutes`: gaining and losing the leadership starts and stops all routes, a quick true/false/true sequence ends with the routes started (and staying started), a route can be removed while the view stays in use, and a CamelContext stop releases the view. New `ClusteredRoutePolicyFactoryTest.testNoPolicyThreadLeftWhenIdleOrRoutesRemoved`: while leader, adds 20 routes (a policy each), waits until they are started and checks that no `ClusteredRoutePolicy` thread of the CamelContext is left while they are idle, then stops and removes them; three times. After that it checks again that no thread is left, and that a leadership loss after the threads have exited still stops the remaining route. Negative controls, with only `ClusteredRoutePolicy.java` swapped: ``` origin/main ClusteredRoutePolicy testCamelContextStopDuringLeadershipChange CamelContext stop deadlocked with the leadership change ==> execution timed out after 20000 ms testRemoveRouteDuringLeadershipChange removeRoute deadlocked with the leadership change ==> execution timed out after 20000 ms testLeadershipChangesStartAndStopRoutes passes the first version of this change (a single thread scheduled executor per policy) testNoPolicyThreadLeftWhenIdleOrRoutesRemoved live ClusteredRoutePolicy threads ==> expected: <0> but was: <21> within 10 seconds this change, but the listener calls setLeader on the dispatch thread (lock-free release only) testRemoveRouteDuringLeadershipChange removeRoute deadlocked with the leadership change ==> execution timed out after 20000 ms ``` With the change, the `ClusteredRoutePolicy*Test` classes (14 tests) passed 4 times in a row (`-Dsurefire.rerunFailingTestsCount=0`). `*Cluster*`, `*RoutePolicy*` and `FileLock*` in camel-core (71 tests, including `ClusteredRoutePolicy*Test` and `FileLockClusteredRoutePolicy*Test`), the whole camel-file suite (22 tests) and the whole camel-master suite (30 tests) pass. `JGroupsRaft*` in camel-jgroups-raft (7 tests) and the cluster tests of camel-infinispan-embedded (`InfinispanEmbeddedClustered{RoutePolicy,RoutePolicyFactory,Master,View}Test`, 4 tests) pass. The change merges cleanly with #26932 (CAMEL-25052, `FileLockClusterView`). Not run locally: the ITs of camel-zookeeper, camel-consul and camel-kubernetes (they need containers). They wait on latches or with Awaitility for the route status. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested the affected modules, including the formatter and import-sort plugins. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
