allthingssecurity opened a new pull request, #27800: URL: https://github.com/apache/camel/pull/27800
# Description [CAMEL-25613](https://issues.apache.org/jira/browse/CAMEL-25613) This is about a remote JMX consumer with `reconnectOnConnectionFailure=true`. The option is not the default; it is the one to set so that the consumer survives an outage of the monitored JVM. Its reconnect task closes the previous connector, connects, and adds the notification listener; on main the connection listener schedules one such task for every `FAILED`, `CLOSED` and `NOTIFS_LOST` notification that carries the connection id the consumer stored when it connected (`JMXConsumer` 190-207 and 213-226 on main). What went wrong: 1. One lost connection scheduled two reconnects, and after that the replacement never stopped. The RMI connector reports a connection that can no longer be used with `FAILED` and then, from the close that follows, with `CLOSED`, both with the id of that connection (observed with JDK 21: `RMIConnector.RMIClientCommunicatorAdmin.gotIOException` sends the first and calls `close(true)`, which sends the second). Both passed the id check (line 193 on main), so two tasks were scheduled. Once the server was back the first task connected and subscribed, and the second closed that working connector (line 142). Closing a connector whose connection works also sends `CLOSED` with the stored id, synchronously, so the listener scheduled the next task, and so on: after an outage the consumer closed and replaced a working connection every `reconnectDelay` for as long as the route ran, with a WARN each time, and notifications sent between a close and the next subscription were lost. The same replac ement started after one `NOTIFS_LOST`, and after an attempt that had connected but could not add the listener because the MBean was not registered yet. 2. The RMI connector re-establishes its connection by itself when the server object is gone but the server still answers. It then reports `OPENED` with a new connection id and adds again only the listeners whose MBean exists at that moment (read in the JDK sources). The consumer ignored that and kept the old id, so it ignored every later connection failure (no reconnect, nothing logged), and it stayed without a subscription when the MBean was registered later. 3. A notification about a lost connection that was handled after the consumer had been stopped created a new scheduled executor and connected the stopped consumer again. Changes, all in `JMXConsumer`: - `scheduleReconnect` schedules at most one reconnect at a time: an `AtomicBoolean` is set with `compareAndSet` when a reconnect is scheduled, cleared when the task begins (so a failure reported during the attempt schedules the next one) and reset in `doStop`. It schedules nothing when the consumer is not allowed to run, and the task ends at once in that case. - `initNetworkConnection` removes the connection listener from the previous connector before it closes it. - An `OPENED` notification with another id: the consumer stores the new id and, with `reconnectOnConnectionFailure=true`, schedules its normal reconnect, which subscribes again on a new connection (it cannot know whether the connector kept its listener). With `reconnectOnConnectionFailure=false` it only logs. - The listener ignores a lost connection when the consumer is not allowed to run. - `mConnector`, `mConnectionId` and `mScheduledExecutor`, which the notification threads read, are volatile. What users can notice: - One new WARN line when the connector reports a connection it opened by itself (in both settings of the option), and, with `reconnectOnConnectionFailure=true`, one reconnect by the consumer `reconnectDelay` after such a notification. When the connector had added all listeners again, the consumer on main kept receiving without a gap; now that connection is replaced once, and notifications that the MBean sends between the close and the new subscription are lost that one time. In exchange the consumer is subscribed for certain and reacts to later failures. - While the server is down there is one reconnect attempt per `reconnectDelay` instead of two. No option, default or message format changes, so there is no upgrade guide entry. No new retry loop: the attempts are those of the existing reconnect task, `reconnectDelay` apart, with the existing WARN line (no stack trace) per failed attempt. Tests: - New `JMXConsumerReconnectTest` (5 tests), with an RMI registry and a connector server in the test JVM set up like the existing `JMXRobustRemoteConnectionTest`, and `reconnectDelay=1`. The connector server counts the client connections it accepts. - `FAILED` and then `CLOSED` with the current connection id are given to the connection listener: one new connection, then none for three seconds, and one more notification of the MBean is delivered once. - A `NOTIFS_LOST` notification for the current connection: one new connection, then none for three seconds. - The server is stopped and started again without the MBean; the test waits for two new connections, registers the MBean and waits for a notification. After that the connections must stop (a period of three seconds without one), and one more notification is delivered once. - An `OPENED` notification with another connection id: the consumer opens a connection of its own again, then none for three seconds. - The route is stopped and `FAILED` with the last connection id is given to the listener: no connection for three seconds. - Without the change (main's `JMXConsumer`, same test class) all five fail: `the consumer connected again although it was subscribed on a working connection ==> expected: <false> but was: <true>` (twice), `the consumer kept connecting although it was subscribed on a working connection`, `the consumer did not subscribe again after the connector had reconnected by itself` and `the consumer connected again although it was stopped`. - A real outage, run locally and not part of this PR: a JMX server in a second JVM is killed and started again on the same port, `reconnectDelay=1`. With main's `JMXConsumer` the connector delivered `FAILED` and `CLOSED` with the stored id, and the new server accepted 16 connections from the consumer in the first 8 seconds (two every second). With the change it accepted one, and the next notification of the MBean was delivered. - With the change, camel-jmx tests pass: 58 run, 0 failures, 0 skipped. What is not changed: - With `testConnectionOnStartup=false`, a delayed start that connects but cannot add the listener still logs "An unrecoverable exception has occurred" at ERROR and does not try again. - `doStop` still fails when the server cannot be reached at that moment. - A reconnect attempt that is already connecting when the consumer is stopped is not interrupted and may leave a connection open, as before. - The WARN "Lost JMX connection" is still logged for each of the two notifications of one lost connection. What was not run: a JMX server on another host (the servers were in the test JVM or in a second JVM on the same machine). That the RMI connector adds again only the listeners whose MBean exists is read from the JDK sources; the test for that case gives the `OPENED` notification to the listener. Notes: - How it was found: a sweep of the reconnect code of the components, then a small TLA+ model of the consumer (server, MBean, connector, stored id, subscription, pending and running reconnect, the `FAILED` and `CLOSED` pair, stop). For main it reaches two pending reconnects after one lost connection, a replaced connection without a cause, and a reconnect scheduled after stop; with the change those properties hold. The model is only a sketch of the code; the tests above are the evidence. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [ ] 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). (The ticket is drafted and not filed yet; CAMEL-25613 is a placeholder until it is.) # 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 camel-jmx, 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]
