allthingssecurity opened a new pull request, #28805: URL: https://github.com/apache/camel/pull/28805
# Description [CAMEL-26449](https://issues.apache.org/jira/browse/CAMEL-26449) `DefaultShutdownStrategy` keeps the state of a shutdown in fields of the strategy: `currentShutdownTaskFuture` (the task the caller waits for, and cancels when its timeout elapses), `timeoutOccurred` (also handed to the `ShutdownTask`, whose wait loop ends when it is set) and `forceShutdown`. Two shutdowns can run at the same time: a route stop or suspend (`stopRoute(id, timeout, unit)`, `suspendRoute`, a route policy, the control bus, a reload) holds the route lock of the CamelContext, while `CamelContext.stop()` (`shutdownForced`) runs under the lifecycle lock, so nothing keeps them apart. The timeout of one then changes the other: - **The route stop starts first, the CamelContext stop after it.** The CamelContext stop overwrites `currentShutdownTaskFuture` with its own task. When the route stop times out it cancels `currentShutdownTaskFuture`, which is now the task of the CamelContext stop (its own task keeps running), and sets `forceShutdown` back to false. `Future.get` in the CamelContext stop throws `CancellationException`, which is handled as a timeout: the CamelContext forces every route to shut down right away (`shutdownRoutesNow`, forced `prepareShutdown`), after the timeout of the route stop instead of its own. Consumers are stopped under exchanges in flight. (`doShutdown` also reads the field again for `get`, so it can wait for the other call's task, or get a `NullPointerException` when the other call has just cleared the field; both are handled as a timeout too.) - **The CamelContext stop starts first, the route stop after it.** The task of the route stop waits behind the task of the CamelContext stop (the executor has one thread), so the route stop times out and sets `timeoutOccurred`. The wait loop of the CamelContext's task reads the same flag and ends: the CamelContext stops all routes while their exchanges are still in flight, and logs no timeout (`Shutting down while there are still 2 inflight exchanges`). Either way, exchanges in flight on the other routes are cut off during what should be a graceful shutdown. A pod that is stopped while a route is being stopped (by a route policy, a cluster leadership change, the control bus or management) gets there. The change keeps the future and the timeout flag of a shutdown in local variables: the call waits for and cancels its own task, and its `ShutdownTask` reads its own flag. The fields are still set for `getCurrentShutdownTaskFuture()` and `isTimeoutOccurred()` (which some components and tests read); the future field is only cleared when it still holds the task of the call (`AtomicReference.compareAndSet`). `forceShutdown` is only ever set to true (by a forced shutdown that times out), so a route stop that times out no longer turns it off while the CamelContext is being forced down; `doStart` still resets it. The assignment `this.forceShutdown = forceShutdown` came with CAMEL-15245 so that a route stop that times out does not leave the flag true (and reject exchanges once the route is started again); that still holds, as a non-forced call never sets it, and only `shutdownForced` (from the CamelContext stop) does. Two more changes for the same case (found while testing this one): - A route stop that times out while the CamelContext is stopping shuts its route down (`RouteService.doStop` with `camelContext.isStopping()`), and `DefaultRoute.doShutdown` clears the services list of the route. The task of the CamelContext stop iterates the same list (to count pending exchanges and to prepare the services), got a `ConcurrentModificationException`, and the CamelContext then stopped all routes without waiting ("Error occurred while shutting down routes. This exception will be ignored."). The services of a route are now a `CopyOnWriteArrayList` (written only when the route is set up or shut down). - The timeout path loops over `routesOrdered` (the copy made at the start) instead of the caller's list, which for a CamelContext stop is the live route startup order list that a concurrent `removeRoute` changes. The forced `prepareShutdown` then runs in shutdown order, the same as `shutdownRoutesNow` and the `ShutdownTask`. Not changed (follow-ups): - The `timeoutOccurred` field is still last-writer-wins for code outside the strategy that reads `isTimeoutOccurred()`: `CxfSpringEndpoint` waits for the shutdown with it, `LoggingExceptionHandler` suppresses logging with it, JMX shows it, and after a graceful CamelContext stop it reads true if a concurrent route stop timed out. The strategy itself no longer uses the field. It cannot simply stop being set by route stops, as tests and users read it after a route stop. - The executor still runs one `ShutdownTask` at a time, so a shutdown whose task waits behind another one spends part of its timeout waiting, or times out without its task running (it then forces its own routes after its own timeout, as before). The executor is also created lazily without a lock, so the first two shutdowns of a CamelContext, at the same time, can create two. - `forceShutdown` is read by every exchange on other threads and is not `volatile` (unchanged, see CAMEL-16348 for why the check is kept cheap). I can file the first two as one follow-up JIRA. ## How it was found With a TLA+ model of `doShutdown` called by two threads at once (`tla/1101c/ShutdownShared.tla` in our verification workspace: two callers, the single-thread executor, the three fields; each step is one step of `doShutdown` or `ShutdownTask.run`). With a CamelContext stop that never times out: - `NoForeignForce` (a caller forces its routes only when its own timeout elapsed) is violated: the CamelContext stop reads the field after the route stop has replaced it, waits for the route's task, and is forced when the route stop times out and cancels that task (a 12-state trace); - `NoEarlyExit` (a task stops waiting with exchanges in flight only when its caller timed out) is violated: the route stop times out, sets the shared flag, and the task of the CamelContext stop leaves its wait loop (a 10-state trace). When the CamelContext stop can time out too, `ForceStays` (a forced CamelContext stop is not turned back into a graceful one) is violated as well: a route stop that times out after it sets `forceShutdown` to false. With local state per call all three hold (1,126 states; 415 when the CamelContext stop never times out), and witness configurations show that a graceful completion of both, and both callers forcing after their own timeout, are still reachable. The model does not have the services list of a route (the second change above was found by repeating the tests). ## Tests `DefaultShutdownStrategyConcurrentShutdownTest` (new) has two routes whose consumers are suspended (not stopped) during a graceful shutdown, and an exchange in flight on each (none on route `a` in the third test). A service of route `a` (`ShutdownAware`) can hold the task of the CamelContext stop while it counts pending exchanges, or the CamelContext stop while it prepares route `a` for a forced shutdown: - `testRouteStopTimeoutDoesNotForceContextStop`: route `a` is stopped with a 5 second timeout; while its task waits, the CamelContext is stopped with a 60 second timeout; the route stop times out. - `testRouteStopTimeoutDoesNotCutShortContextStop`: the CamelContext is stopped first (60 seconds) and waits; then route `a` is stopped with a 1 second timeout, which times out (checked). - `testRouteShutdownDuringContextStopDoesNotFailItsWait`: the task of the CamelContext stop is held (in `getPendingExchangesSize` of a service of route `a`) while it iterates the services of route `a`; meanwhile route `a` is stopped with a 1 second timeout, times out, and shuts route `a` down. - `testRouteStopTimeoutKeepsForcedContextStop`: the CamelContext stop (2 second timeout) times out and forces the routes; while it prepares route `a` for the forced shutdown, a route stop that started before it (10 seconds) times out; `forceShutdown` must stay true. The first three check that the CamelContext keeps waiting for the exchange of route `b` (its consumer stays suspended, the exchange stays in flight, `forceShutdown` stays false) and stops gracefully once the exchanges complete, with the exchange of route `b` completed without an exception. On main the first test fails because the consumer of route `b` is stopped while its exchange is in flight (`forceShutdown` true, 5 seconds after the start); in the second the CamelContext stops 2 seconds in, with 2 exchanges in flight and no timeout warning; the third fails because the CamelContext stops without waiting for route `b` (the log shows the `ConcurrentModificationException`). The third also fails with only the strategy change (the `ArrayList` of main's `DefaultRoute`); before it was added, the first test failed 1 time in 60 runs with only the strategy change for that reason. The fourth passes on main too, because there the shared timeout flag ends the task of the route stop before it s own timeout, so its catch is not reached; it fails when the change keeps `this.forceShutdown = forceShutdown`. With the change all four pass; the test class ran 20 times in one JVM: 80 tests, 0 failures (twice: by us and by our G1 review). The camel-core tests in `org.apache.camel.impl`, the seda and direct components, and every shutdown, graceful, suspend, route-policy and route-stop test ran: 1,129 tests, 8 failures that also happen on main in our local environment (7 in `RouteChecksumTest` and 1 in `SedaConfigureTest`, checked against main's classes); camel-support 3 tests. 5 camel-management tests fail only because our local run forces all test classes into one JVM (shared MBean server); each passes when run alone. The formatter and import-sort plugins ran. No upgrade guide entry: nothing changes for a single shutdown at a time. Backport: the same fields are on 4.14.x, 4.18.x, 4.22.x and 4.23.x, and the commit applies to each; a candidate as it cuts off in-flight exchanges silently, at the maintainers' choice. _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]
