[etzel-mes]

Description

Upgrades camel-plc4x from PLC4X 0.13.1 to 1.0.0 (JIRA: CAMEL-25617). PLC4X 1.0.0 requires Java 21, which Camel main has since CAMEL-22996, and it removed the plc4j-scraper module the triggered consumer was built on.

The user-facing contract is unchanged: the same endpoint options (trigger, period, tags, autoReconnect), the same Map<String,Object> body, the same one-shot consumer, polling consumer and producer. Nothing was added to or removed from the generated endpoint metadata.

Triggered consumer. With the scraper gone, the consumer with trigger is now a Camel ScheduledPollConsumer on the endpoint's single connection:

every `period` ms (fixed delay, first poll at route start)
  reconnectIfNeeded()
  read trigger tag + data tags in ONE request
  if any tag not OK or the read failed
    remembered trigger := unknown; report through the exception handler
  else if trigger is true and remembered trigger was not true
    deliver the data tags as Map<String,Object> (trigger tag excluded)
  remembered trigger := trigger

This keeps the scraper's semantics (rising edge only, a trigger already true on the first read emits once, a held trigger never re-emits) and changes what was broken or implicit: the data tags come from the same read as the edge (the scraper read them in a later separate request on another connection), a failed read no longer stops triggering for the life of the route (the scraper's collector died on the first failure), stop cancels the schedule, and no scraper thread pools remain (each scraper start leaked up to 20 non-daemon threads). The 0.13.1 behaviour was locked by route tests first (commit 1) and the port is proven by the same tests passing (commits 2 and 5); the two Mockito tests that captured the scraper's constructors, including the stop test from CAMEL-25292, are re-expressed at the route seam.

PLC4X 1.0.0 offers plc4j-tools-event-pump in place of the scraper; it is not used here because its cached read request stays bound to the connection that built it, so it never recovers from a PLC restart (apache/plc4x#2832, found while preparing this change). Camel's own polling consumer gives the scheduling, overlap guarantee, stop and error routing without a new dependency.

Other changes. reconnectIfNeeded() now closes a lost connection and takes a new one from the driver manager instead of calling connect() on it; the 1.0.0 S7 and EtherNet/IP connections refuse to connect again once their transport closed, so without this neither the producer nor the consumers recovered after a PLC restart. The endpoint no longer passes its own Camel options (trigger, period, tag.*, autoReconnect, generic consumer options) to the driver inside the connection string; 1.0.0 drivers log a warning per unknown parameter. Constants.PLC_NAME (scraper-only) is removed. The upgrade guide gets a camel-plc4x section with the PLC4X-side changes (Java 21, renamed connection parameters, OPC UA secure defaults, with a link to the PLC4X release notes) and the behaviour changes above.

Not changed, for the record. With the default autoReconnect=false the endpoint stops itself on the first lost connection while the route keeps polling; that is pre-existing and shared with the producer. plc4j-tools-event-pump still arrives transitively through plc4j-utils-subscription-emulation (ab-eth, eip, firmata) with the module's long-standing jackson/woodstox exclusions applying under it; the EtherNet/IP path was run with it.

Verification. Module build only (mvn install in components/camel-plc4x; the root build was not run). Module suite: 35 tests, green in three consecutive runs with -Dsurefire.rerunFailingTestsCount=0; -Psourcecheck clean; the catalog copy of the component doc is byte-identical. Live, against protocol simulators with writable memory (S7: snap7 1.4.2 server; Modbus TCP: oitc/modbus-server; EtherNet/IP: libplctag 2.7.2 ab_server as ControlLogix), a producer route wrote the trigger bit and the data tags while a triggered consumer polled at period=100: on all three, a false→true edge delivers exactly one exchange whose body holds the values written just before the edge, a trigger held for 2.5 s delivers no second exchange, 20 edges at 1 Hz delivered 20 matching bodies, poll spacing was 105–109 ms mean (max 132 ms), the JVM used about 2 % CPU, a 10 s simulator stop logged one warning per poll and the consumer recovered 0.1–0.6 s after the simulator came back; the unknown-parameter warnings are gone. Not verified against real PLCs.

Backport. Not backportable to 4.22.x: PLC4X 1.0.0 requires Java 21.

@davsclaus @oscerd could you have a look? (I cannot request reviewers.) JIRA user: mardonius.

Target

• 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 filed for the change (usually before you start working on it).

Apache Camel coding standards and style

• 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.

AI-assisted contributions

• 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.

The code, tests and documentation were written with Claude Code (Claude Fable 5.1) under my direction; every commit carries the Co-Authored-By trailer.

🤖 Generated with Claude Code

Claude Code on behalf of etzel-mes

[github-actions]

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

• First-time contributors require MANUAL approval for the GitHub Actions to run
• You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
• You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
• Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

[gnodet-bot]

LGTM. The scraper-based triggered consumer is cleanly replaced by a ScheduledPollConsumer implementation that correctly handles the rising-edge semantics, polls trigger and data tags in a single request, and properly manages connection lifecycle. The new Plc4XConsumerRouteTest covers the critical paths at the route seam (rising edge, held trigger, stop/restart without duplicates, failure recovery, reconnection, validation). The endpoint option stripping from the connection string is well-guarded and tested. No production-impacting issues found.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[github-actions]

🧪 CI tested the following changed modules:

• catalog/camel-catalog
• components/camel-plc4x
• docs
• parent

✅ POM dependency changes: targeted tests included

Changed properties: plc4x-version

Modules affected by dependency changes (1)

• :camel-plc4x

🔁 1 test passed only after a retry on JDK 21 (1 retried attempt)

Recovered flaky tests on JDK 21 (1)

Module	Test	Failed attempts	First failure
dsl/camel-jbang/camel-jbang-plugin-tui	MonitorStartupTest.browserOnlyServesUntilTheServerStops	1	Connection refused

ℹ️ These tests did not fail the build. Surefire retried them and they passed.
Retries are enabled project-wide by surefire.rerunFailingTestsCount in parent/pom.xml.

---

🔬 Scalpel shadow comparison — Scalpel: 9 of 707 tested, 3 compile-only — current: 8 all tested

Maveniverse Scalpel detected 9 affected modules (current approach: 8).

Changed properties: plc4x-version

Skip-tests mode would test 9 modules (4 direct + 0 downstream), skip tests for 3 (generated code, meta-modules)

⚠️ Modules only in Scalpel (1)

• camel-jpa

Modules Scalpel would test (9)

• camel-jbang-mcp ← depends on affected reactor module org.apache.camel:camel-yaml-dsl-validator
• camel-jbang-plugin-mcp ← depends on affected reactor module org.apache.camel:camel-jbang-core
• camel-jbang-plugin-tui ← depends on affected reactor module org.apache.camel:camel-yaml-dsl-validator
• camel-jbang-plugin-validate ← depends on affected reactor module org.apache.camel:camel-yaml-dsl-validator
• camel-jpa ← effective property camel.surefire.fork.additional-vmargs
• camel-launcher-container ← depends on affected reactor module org.apache.camel:camel-launcher
• camel-plc4x ← components/camel-plc4x/src/main/docs/plc4x-component.adoc, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Constants.java, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XConsumer.java, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XEndpoint.java, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XPollingConsumer.java, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XProducer.java, components/camel-plc4x/src/main/java/org/apache/camel/component/plc4x/Plc4XTriggeredConsumer.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/MockDriver.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XConsumerRouteTest.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XConsumerStopTest.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XConsumerTest.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XEndpointTest.java, components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XFailureTest.java, own pom components/camel-plc4x/pom.xml changed
• camel-yaml-dsl-validator ← depends on affected reactor module org.apache.camel:camel-catalog
• camel-yaml-dsl-validator-maven-plugin ← depends on affected reactor module org.apache.camel:camel-yaml-dsl-validator

Modules with tests skipped (3)

• camel-itest
• camel-yaml-dsl
• camel-yaml-dsl-deserializers

ℹ️ Shadow mode — Scalpel observes but does not affect test execution. Learn more

All tested modules (36 modules, 7m 51s total)

Total reactor time: 7m 51s

Module	Duration	Status
Camel :: Launcher	59.2s	SUCCESS
Camel :: JPA	54.5s	SUCCESS
Camel :: JBang :: MCP	49.3s	SUCCESS
Camel :: PLC4X	35.7s	SUCCESS
Camel :: YAML DSL :: Validator	30.6s	SUCCESS
Camel :: JBang :: Plugin :: Validate	30.3s	SUCCESS
Camel :: Catalog :: Camel Catalog	25.8s	SUCCESS
Camel :: YAML DSL	23.1s	SUCCESS
Camel :: Component DSL	22.5s	SUCCESS
Camel :: JBang :: Plugin :: Testing	21.4s	SUCCESS
Camel :: Kamelet Main	19.6s	SUCCESS
Camel :: JBang :: Plugin :: Kubernetes	18.1s	SUCCESS
Camel :: Docs	16.4s	SUCCESS
Camel :: YAML DSL :: Deserializers	9.3s	SUCCESS
Camel :: Catalog :: Camel Report Maven Plugin	8.3s	SUCCESS
Camel :: Catalog :: Camel Route Parser	8.3s	SUCCESS
Camel :: Assembly	5.7s	SUCCESS
Camel :: All Components Sync point	5.4s	SUCCESS
Camel :: YAML DSL :: Validator Maven Plugin	4.0s	SUCCESS
Camel :: Catalog :: Maven	4.0s	SUCCESS
Camel :: Coverage	3.3s	SUCCESS
Camel :: YAML DSL :: Maven Plugins	3.3s	SUCCESS
Camel :: Catalog :: Suggest (deprecated)	3.1s	SUCCESS
Camel :: JBang :: Plugin :: Edit	1.4s	SUCCESS
Camel :: JBang :: Plugin :: MCP	1.3s	SUCCESS
Camel :: JBang :: Integration tests	1.2s	SUCCESS
Camel :: JBang :: Plugin :: Generate	1.1s	SUCCESS
Camel :: Catalog :: Dummy Component	1.1s	SUCCESS
Camel :: Catalog :: Console	1.1s	SUCCESS
Camel :: JBang :: Main	1.0s	SUCCESS
Camel :: Endpoint DSL :: Support	0.8s	SUCCESS
Camel :: Launcher :: Container	0.5s	SUCCESS
Camel :: Endpoint DSL	n/a
Camel :: Integration Tests	n/a
Camel :: JBang :: Core	n/a
Camel :: JBang :: Plugin :: TUI	n/a

Top 20 slowest modules:

• Camel :: Launcher (59.2s)
• Camel :: JPA (54.5s)
• Camel :: JBang :: MCP (49.3s)
• Camel :: PLC4X (35.7s)
• Camel :: YAML DSL :: Validator (30.6s)
• Camel :: JBang :: Plugin :: Validate (30.3s)
• Camel :: Catalog :: Camel Catalog (25.8s)
• Camel :: YAML DSL (23.1s)
• Camel :: Component DSL (22.5s)
• Camel :: JBang :: Plugin :: Testing (21.4s)
• Camel :: Kamelet Main (19.6s)
• Camel :: JBang :: Plugin :: Kubernetes (18.1s)
• Camel :: Docs (16.4s)
• Camel :: YAML DSL :: Deserializers (9.3s)
• Camel :: Catalog :: Camel Report Maven Plugin (8.3s)
• Camel :: Catalog :: Camel Route Parser (8.3s)
• Camel :: Assembly (5.7s)
• Camel :: All Components Sync point (5.4s)
• Camel :: YAML DSL :: Validator Maven Plugin (4.0s)
• Camel :: Catalog :: Maven (4.0s)

---

⚙️ View full build and test results

[davsclaus]

TL;DR

Comment: the port to PLC4X 1.0.0 is clean and well tested, and the existing upgrade guide notes are right. Two 1.0.0 changes that break user routes are missing from the upgrade guide.

Blockers

• None in the code. I would like the upgrade guide (camel-4x-upgrade-guide-4_24.adoc, near line 2651) to cover these before merge:
  • Tag addresses: as far as I can see, 1.0.0 writes array selection as a range before the data type (holding-register:1:INT[4] → holding-register:1[0..3]:INT, %DB42:28.0:BYTE[4] → %DB42:28.0[0..3]:BYTE), and the old form no longer parses. That affects the tag.* options, trigger and the producer body. Please check and add a sentence with an example.
  • s7-light is gone: plc4j-driver-s7 0.13.1 registered S7Driver and S7LightDriver, and 1.0.0 registers only S7Driver. So plc4x:s7-light:// stops working, and the PLC4X release notes don't say so.

Other findings: 3 non-blocking, 2 suggestions. See below.

Non-blocking

• Plc4XTriggeredConsumer.java:88: Boolean.TRUE.equals(response.getObject(TRIGGER)) is false for an INT that reads 1, or for an array, which 1.0.0 now returns on OPC UA/ADS for an address with no selection. The consumer then polls forever without delivering or logging anything. Failing the poll when the value is not a Boolean would make that visible.
• Plc4XEndpoint.java:155: reconnectIfNeeded() now closes and replaces the connection, and the producer calls it for every exchange without a lock. Two senders can each replace it, and one connection leaks or gets closed under the other's write. Make reconnectIfNeeded()/setupConnection() synchronized and the field volatile.
• With autoReconnect=false, after a lost connection the triggered consumer logs the "auto-reconnect is turned off" WARN plus a stack trace on every poll (Plc4XEndpoint.java:165/322, Plc4XTriggeredConsumer.java:116). Before, the scraper just went quiet. Logging once, or stopping polling, could be a follow-up.

Suggestions

• Plc4XTriggeredConsumer.java:122: .execute().get() has no timeout, unlike the producer's get(5000, ms).
• The note at line 2660 that the route fails to start could add that this fails the CamelContext start too, unless the route is supervised or autoStartup=false.
• 1.0.0 brings jackson-dataformat-xml in via plc4j-tools-event-pump. Could you paste a before/after mvn dependency:tree to show the exclusions still hold?

CI: "PR doc validation" fails on cbor-dataformat.adoc:63. That is a problem on main, not this PR.

Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

[davsclaus]

Closed and reopened only to get a new CI run: "PR doc validation" failed on cbor-dataformat.adoc:63, a problem on main that #28388 fixed. Nothing for you to change for that.

Claude Code on behalf of davsclaus

[etzel-mes]

Thanks @davsclaus, all points checked against the 0.13.1 and 1.0.0 jars. Pushed 98c1acd, d5eef55 and bd9fbb9.

Upgrade guide (bd9fbb9):

• Tag addresses: confirmed. The 1.0.0 driver patterns put the array selection before the data type, and the old form no longer parses. The guide now gives holding-register:1:INT[4] → holding-register:1[0..3]:INT and %DB42:28.0:BYTE[4] → %DB42:28.0[0..3]:BYTE, and notes that [4] before the type selects the single element at index 4 (ArrayNotationParser in plc4j-spi-drivers 1.0.0). It covers the tag.* options, trigger and the producer body.
• s7-light: confirmed from the PlcDriver services file (0.13.1 lists S7Driver and S7LightDriver, 1.0.0 only S7Driver). The guide says plc4x:s7-light:// no longer finds a driver and to use plc4x:s7://.
• The failed-start note now says it also fails the CamelContext start, unless the route is supervised or has autoStartup=false.

Non-blocking / suggestions:

• Non-boolean trigger (d5eef55): a trigger value that is not a Boolean (a number, an array, or no value) now fails the poll with a PlcException naming the trigger address and the value type, so it reaches the consumer's exception handler and the remembered trigger is reset. New route test aTriggerThatIsNotABooleanFailsThePoll.
• Timeout (d5eef55): the triggered read now waits at most 5 s, the same value as the producer, through one constant Plc4XEndpoint.REQUEST_TIMEOUT_MILLIS; on timeout the future is cancelled and the poll fails.
• Connection replacement (98c1acd): connection is volatile, and the replacement in reconnectIfNeeded()/setupConnection() and the close in doStop() run under the endpoint lock. I used a synchronized (this) block rather than synchronized methods so that the autoReconnect=false branch calls stop() after releasing the lock; otherwise a concurrent stop(), which holds the service lock and waits for the endpoint lock in doStop(), could deadlock with it.
• The repeated WARN with autoReconnect=false I'd leave for a follow-up, as you suggested.

Dependency tree, components/camel-plc4x, merge base dc99092 (PLC4X 0.13.1) vs this branch:

Artifact	before (0.13.1)	after (1.0.0)
plc4j-scraper	compile (direct)	absent
plc4j-tools-event-pump	absent	runtime (driver → plc4j-utils-subscription-emulation)
jackson-core, jackson-annotations, jackson-dataformat-yaml	compile (via scraper)	runtime (via event-pump)
jackson-databind	compile (via scraper)	absent (exclusion holds)
jackson-dataformat-xml	absent	runtime (via event-pump)
woodstox-core	absent	absent (exclusion holds)

So the exclusions still hold and no Jackson artifact is in compile scope any more. The module no longer declares event-pump; it comes in at runtime through the drivers' subscription emulation, which only uses EventPump, TagBatch, TimerTrigger and Trigger, none of which touch Jackson (only event-pump's config package does).

Module build with tests and -Psourcecheck: 36 tests, 0 failures.

Claude Code on behalf of etzel-mes

[gnodet-bot]

Following up on my earlier review — all previous issues and davsclaus's blockers are addressed in the 3 new commits.

The three commits cover: tag address format and s7-light removal in the upgrade guide, non-boolean trigger detection (fails the poll with a PlcException naming the address and value type), a 5 s timeout on the triggered read via the shared REQUEST_TIMEOUT_MILLIS constant, and the connection lifecycle refactor (volatile field, synchronized blocks in setupConnection/reconnectIfNeeded/doStop, closeLostConnection, stop() called outside the lock). All of davsclaus's blockers and non-blocking items are addressed.

One observation, not a blocker: Plc4XTriggeredConsumer.read() checks plc4XEndpoint.connection == null and then calls plc4XEndpoint.buildPlcReadRequest(...) (which dereferences connection) without holding the endpoint lock between the two. A concurrent doStop() that nulls connection under the lock could slip in between them and cause an NPE on connection.readRequestBuilder(). This is the same pattern carried by Plc4XConsumer, Plc4XPollingConsumer, and Plc4XProducer today, so it is component-wide and not introduced by this PR. In practice ScheduledPollConsumer runs the poll to completion before the executor shuts down during stop, so the window is effectively closed at runtime. Worth a follow-up cleanup, but not a reason to hold this PR.

The 14 route-level tests in Plc4XConsumerRouteTest cover the contract thoroughly: rising edge detection, data delivery without the trigger key in the body, stop/restart without duplicates, error recovery (failed reads, non-boolean trigger, error response codes, refused connections), reconnection after dropped connections, and driver parameter stripping. The upgrade guide section is accurate and complete.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[etzel-mes]

Following up on the real hardware remark on CAMEL-25617: results for bd9fbb9 on a real PLC.

Setup: a Siemens S7-1200 (CPU 1214C, 6ES7 214-1AG40-0XB0) on a customer test line, reached from a Windows Server 2025 machine on the plant network. The connection string was s7://<plc>:102?controller-type=S7_1200&cotp.remote-rack=0&cotp.remote-slot=1&pdu-size=240. A small camel-main jar with this branch's camel-plc4x and the PLC4X 1.0.0 S7 driver ran the read, write-back and trigger checks on a Java 21 JRE on that machine. The remaining checks ran on a second machine (Java 25) through an SSH tunnel to the PLC, which also let me cut or stall the connection on purpose. Only the tags of one idle line were written.

Check	Result
Periodic read (REAL, two BOOLs), 1 s, 40 reads	all OK; a value changed by hand during the run showed up on the next read
Data types, one request each	BYTE, WORD, INT, DINT, REAL, DWORD, BYTE array, %MB, %IB, %QB, %MW all OK
Write the same values back, one request per tag and REAL + BOOL in one request	all OK, values unchanged
Write 123.45, -1.5, 0, 3.4E38, 1.0E-38 to a REAL	read back bit-identical
Triggered consumer (BOOL trigger, period 500 ms), 3 pulses of 2 s	exactly 3 exchanges, 70-90 ms after each rising edge
Same endpoint for 60 s: triggered consumer (100 ms) + producer writing every 50 ms + a pulse every 1 s	60 pulses → 60 exchanges, 821 writes, 360 polls, 0 failures, no new connection under load
Short pulses at period 100 ms	150 ms and longer: 5/5 each; 100 ms and 50 ms: 4/5 (expected for a polled trigger)
REAL tag as trigger	poll fails within 70 ms with "must read as a Boolean, but read a java.lang.Float", no exchange
%DB999.DBD0, %DB1.DBD60000, %DB1.DBX16.9, garbage	INVALID_ADDRESS each; the connection stays usable
Connection cut for 12 s (autoReconnect=true)	each poll fails with "Connection refused", reconnects about 1 s after the PLC is reachable again, next pulse delivered
PLC silent for 15 s (TCP open, no answers)	polls fail after the 5 s request timeout; the late answer is dropped ("unknown tpduRef"); no false trigger, next pulse delivered
controller-type=ANY / pdu-size=1024	the SZL probe identifies the S7-1200; the PDU is negotiated down to 240

Things I saw that are outside this PR, for separate tickets if you think they are worth it:

• setupConnection() replaces the endpoint's connection without closing the old one. The producer and the triggered consumer both call it on start, so a route with both holds two PLC connections, which matters on an S7-1200.
• With autoReconnect=true, every failed poll logs a WARN with the full stack trace, every period.
• The PLC4X 1.0.0 S7 driver fails to decode a BOOL array reply (%DB1.DBX16.0[0..7]:BOOL → INTERNAL_ERROR, BufferUnderflowException). It is the same on the real PLC and on snap7, so it belongs to PLC4X, not to camel-plc4x.

[davsclaus]

TL;DR

Approve: thanks for the three follow-up commits and for testing on a real S7-1200. Every point of my earlier review is handled, and the upgrade guide now covers the two route-breaking changes of PLC4X 1.0.0.

Blockers

• None

Other findings: 2 non-blocking follow-ups, which you already named yourself. See below.

Earlier points

Earlier point	Status
Upgrade guide: array notation of tag addresses (holding-register:1:INT[4] → holding-register:1[0..3]:INT)	Done (camel-4x-upgrade-guide-4_24.adoc:2678), incl. that [4] before the type now means one element
Upgrade guide: s7-light driver is gone	Done (camel-4x-upgrade-guide-4_24.adoc:2684)
Non-boolean trigger polled forever without a sign	Done: Plc4XTriggeredConsumer.read() fails the poll with the address and the type, new test aTriggerThatIsNotABooleanFailsThePoll
reconnectIfNeeded() replaced the shared connection without a lock	Done: volatile field, replace/setup/close under the endpoint's lock, and stop() outside it, so it cannot deadlock with a concurrent stop()
.execute().get() without a timeout	Done: REQUEST_TIMEOUT_MILLIS shared with the producer, the future is cancelled on timeout
Failed start also fails the CamelContext start	Done, with a link to the supervising route controller
Jackson exclusions after plc4j-tools-event-pump	Done: the before/after dependency tree in the PR shows jackson-databind and woodstox-core still excluded, nothing Jackson in compile scope
Repeated WARN with autoReconnect=false	Left for a follow-up, as agreed

CI on this head: camel-plc4x passes (36 tests) and the generated files are in sync. "Build and test" fails only on SourceSizeTest in camel-jbang-plugin-tui (SourceTab.java has 2007 lines). That was a problem on main, fixed by #28419 after this run. A re-run keeps the old merge commit, so the branch needs an update from main to go green. GitHub cannot update it automatically because it now conflicts with main, so could you rebase it onto main?

Non-blocking (follow-ups)

• Plc4XEndpoint.setupConnection() (line 132) still replaces connection without closing a previous one. This PR adds a new caller: the triggered consumer calls it in doStart(). So a route stop/start, or a producer and a triggered consumer on the same endpoint, leaves one open PLC connection behind (you saw this too). Now that the method holds the lock, a guard there would be a small change: keep a connection that is still connected, and close a lost one first (closeLostConnection()).
• Plc4XEndpoint.doStop() (line 335) closes the connection only if (isConnected()), so a lost connection is never closed or cleared on stop. closeLostConnection() could do this too. This was already the case before the PR.

The open gnodet-bot thread on Plc4XTriggeredConsumer.java:119 (the null check and the use of connection are not one step) is a pattern the whole component already has, so it fits the same follow-up.

This review checks the Camel project conventions. It does not replace static analysis or dedicated review tools.

---

Reviewed with Claude Code on behalf of Claus Ibsen (@davsclaus). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

[davsclaus]

Thanks @etzel-mes. The red build (21, false) is not caused by this PR. It is SourceSizeTest.noFileOrMethodGrowsOverTheLimits in camel-jbang-plugin-tui (SourceTab.java has 2007 lines), which was broken on main at the time of the run and was fixed by #28419. The branch now conflicts with main. Could you rebase it onto main, so CI runs green before merge? My approval stands.

Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

[gnodet-bot]

Re-review after the latest commits (d5b7005).

All three previously flagged concurrency issues remain present. The PR otherwise looks good — version upgrade is clean, the synchronization work is a real improvement over the original, and no new issues were introduced.

Status of prior findings:

1. setupConnection() — assigns connection without closing the prior lost connection first (follow-up from davsclaus). Accepted as non-blocking.
2. doStop() — a dropped-transport connection (non-null but isConnected() == false) is never closed or nulled. Accepted as non-blocking.
3. TOCTOU in Plc4XTriggeredConsumer.read() — plc4XEndpoint.connection read outside any lock.

All three fit the same follow-up cleanup. The PR is mergeable as-is once davsclaus's requested rebase is done.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after the latest commits (d5b7005).

All three previously flagged concurrency issues remain present. The PR otherwise looks good — version upgrade is clean, the synchronization work is a real improvement over the original, and no new issues were introduced.

Status of prior findings:

1. setupConnection() — assigns connection without closing the prior lost connection first (follow-up from davsclaus). Accepted as non-blocking.
2. doStop() — a dropped-transport connection (non-null but isConnected() == false) is never closed or nulled. Accepted as non-blocking.
3. TOCTOU in Plc4XTriggeredConsumer.read() — plc4XEndpoint.connection read outside any lock.

All three fit the same follow-up cleanup. The PR is mergeable as-is once davsclaus's requested rebase is done.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[davsclaus]

Thanks for the rebase, @etzel-mes. CI is green now (camel-plc4x: 36 tests), and the range-diff against the series I looked at last time shows only context changes from main, so everything from the earlier rounds still holds.

I'd like to take back one thing I said last time. I called the two connection leaks follow-ups, but this PR already rewrites both of those spots, and each fix is a couple of lines. Doing them here saves another round on the same lines later. Also, I was wrong that the triggered consumer is a new caller of setupConnection(): on main, Plc4XConsumer.doStart() already called it for the triggered path. So the leak is older than this PR, but it is still on lines the PR changes.

• Plc4XEndpoint.setupConnection() (around line 147) overwrites connection without closing the old one. Endpoints are not stopped when a route stops, so every route stop/start of a plc4x consumer, and a producer and consumer on the same endpoint, leaves an open PLC connection behind, and S7 connection slots are limited. Inside the lock, could we keep a connection that is still connected and replace only a lost one?

  synchronized (this) {
      if (!isConnected()) {
          closeLostConnection();
          connection = plcDriverManager.getConnection(connectionString);
      }
  }

  The isConnected() guard matters: always closing first would close a live connection that the producer or another consumer is in the middle of using. Asserting that MockDriver.connectionStrings() has size 1 after the restart in stopsReadingWhenTheRouteStopsAndResumesWithoutDuplicatesAfterARestart would lock this in.
• Plc4XEndpoint.doStop() (around line 349) closes only if (isConnected()), so a dropped connection is never closed or cleared. Calling closeLostConnection() there instead would cover both cases, and it also keeps a failing close() on a dead transport from failing the stop.
• Optional, while you're in there: Plc4XTriggeredConsumer.read() (lines 119-123) checks plc4XEndpoint.connection for null and then buildPlcReadRequest() reads the field again, so a concurrent close in between would surface as an NPE instead of "Cannot establish connection". Taking a snapshot in buildPlcReadRequest(Map) and throwing there when it is null would fix it for all four callers.

With those, the open gnodet-bot threads are covered too.

---

Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

[etzel-mes]

Thanks @davsclaus, all three are in 712fa4a.

• setupConnection() now keeps a connection that is still connected and closes and replaces only a lost one, inside the lock as you suggested. stopsReadingWhenTheRouteStopsAndResumesWithoutDuplicatesAfterARestart asserts MockDriver.connectionStrings() has size 1 after the restart; with the guard removed it fails with 2.
• doStop() now always closes and clears the connection. Since that helper now also closes a live connection, I renamed closeLostConnection() to closeConnection(). Plc4XEndpointTest.doStopClosesALostConnection replaces the old test that asserted a disconnected connection is left alone, and its close() throws to show the stop still succeeds.
• buildPlcReadRequest(Map) reads the field once and throws PlcConnectionException("Cannot establish connection") when it is null, so both overloads now declare it. The duplicate null checks in Plc4XConsumer, Plc4XPollingConsumer and Plc4XTriggeredConsumer are gone, and so are the mock connections Plc4XFailureTest set only to get past them.

Module build with tests and -Psourcecheck: 37 tests, 0 failures.

Claude Code on behalf of etzel-mes

[gnodet-bot]

Following up on my earlier review — all three points from davsclaus's CHANGES_REQUESTED are addressed.

The latest commit covers exactly what was asked:

1. setupConnection() guard — the synchronized (this) block keeps a live connection and calls closeConnection() before replacing a lost one. stopsReadingWhenTheRouteStopsAndResumesWithoutDuplicatesAfterARestart asserts connectionStrings().size() == 1 after restart, locking in the behaviour.

2. doStop() closes lost connections — now calls closeConnection() under synchronized (this) unconditionally. closeConnection() tries connection.close(), swallows any exception from a dead transport, and sets connection = null regardless. doStopClosesALostConnection verifies this with a mock that throws on close().

3. TOCTOU in buildPlcReadRequest — the field is read once into a local current; a null current throws PlcConnectionException("Cannot establish connection") before touching anything; all subsequent accesses use current. The inline null checks in the four consumer/producer callers are correctly removed. buildPlcReadRequestWithoutAConnection covers the null path.

canWrite() and isConnected() also take local snapshots now, which is correct. The stop() call in reconnectIfNeeded() is deliberately outside the lock to avoid a deadlock with a concurrent stop() that holds the service lock while doStop() waits for the endpoint lock — the comment explains this.

LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ Follow-up: buildPlcWriteRequest() missing the local-snapshot pattern

Apologies for the oversight in my earlier LGTM — I missed this one.

The PR correctly fixed buildPlcReadRequest(), canWrite(), and isConnected() to read connection into a local variable before use:

PlcConnection current = connection;
if (current == null) { throw new PlcConnectionException(...); }

But buildPlcWriteRequest() (line 297 of Plc4XEndpoint.java) was not updated and still dereferences the volatile field directly:

public PlcWriteRequest buildPlcWriteRequest(Map<String, Map<String, Object>> tags) {
    PlcWriteRequest.Builder builder = connection.writeRequestBuilder();  // ← data race

Plc4XProducer.process() calls reconnectIfNeeded() (synchronized), then isConnected()/canWrite() (both safe with local snapshots), and then buildPlcWriteRequest() — but between the canWrite() check and the writeRequestBuilder() call, a concurrent doStop() can null connection under its own synchronized(this) block, causing an NPE.

Suggested fix (same pattern as buildPlcReadRequest):

public PlcWriteRequest buildPlcWriteRequest(Map<String, Map<String, Object>> tags) throws PlcConnectionException {
    PlcConnection current = connection;
    if (current == null) {
        throw new PlcConnectionException("Cannot establish connection");
    }
    PlcWriteRequest.Builder builder = current.writeRequestBuilder();
    // ... rest unchanged

---

ℹ️ Minor note (pre-existing, non-blocking): getTags() returns the internal Map<String, String> directly, making it mutable from the outside. The new Plc4XTriggeredConsumer.doStart() only calls containsKey() on it, so there is no active bug — but returning Collections.unmodifiableMap(tags) or Map.copyOf(tags) would make the API contract explicit.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[etzel-mes]

@gnodet thanks, the write path is fixed in a214ef9: buildPlcWriteRequest(Map) now reads connection once and throws PlcConnectionException("Cannot establish connection") when it is null, like buildPlcReadRequest(Map). Besides the race with doStop(), this also replaces the NullPointerException a producer got when reconnectIfNeeded() had stopped the endpoint with autoReconnect=false. Plc4XEndpointTest.buildPlcWriteRequestWithoutAConnection covers it.

I left getTags() as it is. It returns the option field like the other endpoint getters, and the generated Plc4XEndpointConfigurer reads the option through it, so I would rather not change it in this PR.

Module build with tests and -Psourcecheck: 38 tests, 0 failures. The branch was also rebased onto main (afa62bc) to resolve a conflict in the 4.24 upgrade guide; both sections are kept.

Claude Code on behalf of etzel-mes

[gnodet-bot]

Following up on my earlier review — the buildPlcWriteRequest() finding is now fixed.

The new commit (a214ef9) correctly applies the local-snapshot pattern to buildPlcWriteRequest(): PlcConnection current = connection is read once, a null check throws PlcConnectionException, and current.writeRequestBuilder() is used for the builder — matching what buildPlcReadRequest() already did. The new unit test buildPlcWriteRequestWithoutAConnection directly validates the null-connection path.

Two review threads opened in my earlier re-review (setupConnection() connection leak) target code that was already fixed in the preceding commits (the synchronized (this) { closeConnection(); connection = ... } pattern is present in setupConnection() in the current diff). They appear non-outdated because the file was subsequently touched by this commit, but the concern is addressed.

LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Following up on my earlier review — all previously flagged issues are now fully addressed.

buildPlcWriteRequest() missing local-snapshot pattern (flagged in my last comment) — ✅ Fixed in the latest commit (a214ef9ebc): reads connection into a local variable, null-checks it, and builds the write request on the snapshot. The companion test buildPlcWriteRequestWithoutAConnection confirms the PlcConnectionException path.

Three concurrency issues from the prior CHANGES_REQUESTED (now resolved across the previous commits):

1. setupConnection() connection leak — ✅ Fixed: synchronized(this) block calls closeConnection() before obtaining a new connection. stopsReadingWhenTheRouteStopsAndResumesWithoutDuplicatesAfterARestart asserts a single connection string after restart.
2. doStop() drops a disconnected connection’s OS resources — ✅ Fixed: synchronized(this) { closeConnection(); } now runs unconditionally, and closeConnection() swallows transport-level close failures. Test renamed to doStopClosesALostConnection and verifies close() is called even when isConnected() is false.
3. TOCTOU on Plc4XTriggeredConsumer: reconnectIfNeeded() → buildPlcReadRequest() gap — ✅ Addressed: connection is volatile, and buildPlcReadRequest(additionalTags) reads it into a local snapshot before the null-check and builder call. The volatile guarantees visibility of the post-reconnect write; the local snapshot prevents use-after-null if a concurrent doStop() clears the field after the snapshot.

The PR is clean. LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[davsclaus]

TL;DR

Approve: the two connection leaks and the null-check race are fixed on the lines this PR rewrites, with tests. This replaces my "changes requested".

Blockers

• None

Other findings: none new.

Thanks @etzel-mes, and thanks for taking these in this PR rather than a follow-up. Re-checked at a214ef9. The range-diff against d5b7005 shows the first eight commits unchanged apart from one context line in the upgrade-guide commit (the rebase), plus the two new commits.

Point	Status
setupConnection() overwrote a connection without closing it	Done: inside the lock it keeps a connection that is still connected and closes and replaces only a lost one. stopsReadingWhenTheRouteStopsAndResumesWithoutDuplicatesAfterARestart asserts a single connection after the restart
doStop() left a dropped connection open	Done: closeConnection() always closes and clears it, and a failing close() no longer fails the stop (doStopClosesALostConnection)
Null check and use of connection were two steps	Done: buildPlcReadRequest(Map) and, after gnodet-bot's catch, buildPlcWriteRequest(Map) read the field once and throw PlcConnectionException when it is null. The duplicate checks in the three consumers are gone, and both paths have a test

A note, no action needed: buildPlcReadRequest() and buildPlcWriteRequest(Map) are public and now declare a checked PlcConnectionException. Only camel-plc4x calls them, and the change replaces an NPE with a clear error, so that's fine.

Still open from your own list, for separate tickets if you want them: the WARN with a stack trace on every failed poll, and the PLC4X 1.0.0 S7 BOOL array decoding error (an upstream PLC4X issue).

This was a static re-review; it doesn't replace CI or static analysis.

Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

[davsclaus]

@etzel-mes, about the S7 BOOL array decoding error you found while testing (%DB1.DBX16.0[0..7]:BOOL → INTERNAL_ERROR, BufferUnderflowException): PLC4X already tracks it as apache/plc4x#2742, and I added your S7-1200 and snap7 findings there (apache/plc4x#2742 (comment)). That issue only has the NPE trace from an S7-1500. If you still have the BufferUnderflowException stack trace, could you add it to #2742? Someone there has just started on a fix, and the trace would show them the second decoding path.

The repeated WARN with autoReconnect is now CAMEL-26339.

Claude Code on behalf of davsclaus