shaipan commented on PR #22874:
URL: https://github.com/apache/camel/pull/22874#issuecomment-4357977732
> Thanks for the follow-up to #22840 — the race condition fix and reconnect
advice improvements are directionally good. A few items to address before this
can be merged:
>
> ### 1. Missing JIRA ticket
> Per project conventions, PRs should reference a JIRA ticket with the
format `CAMEL-XXXX: Brief description` in the title and branch name. Please
create a JIRA ticket and update the PR title/branch accordingly.
>
> ### 2. No tests for changed behavior
> The PR changes concurrency semantics (lock + snapshot) and reconnect
advice handling (multiple new branches), but adds no tests. The existing
`SubscriptionHelperTest` only covers replay ID logic.
>
> At minimum, the reconnect advice logic could be unit-tested by invoking
the connection listener with mocked messages carrying different advice values
(`"none"`, `"handshake"`, `"retry"`, `null`).
>
> ### 3. Incomplete lock coverage — potential lost update on
`channelsToSubscribe`
> The new `channelsLock` protects the compound operations in the handshake
and connection listeners, but `subscribe()` at line 451 calls
`channelsToSubscribe.add(channelName)` under a _different_ lock (`lock`), not
`channelsLock`. This creates a window where `subscribe()` adds a channel after
the snapshot is taken but before `clear()` in the connection listener — the
channel is added, then immediately cleared, and never subscribed.
>
> Similarly, `channelsToSubscribe.remove()` at lines 204 and 266 is
unprotected, though those are benign (removing from a cleared set is a no-op).
>
> Consider either:
>
> * Using `channelsLock` consistently for all mutations of
`channelsToSubscribe`, or
> * Documenting why the partial coverage is sufficient.
>
> ### 4. Behavioral change when advice is null on failed connect
> The old code handshakes when `advice == null` OR `reconnect == "none"`:
>
> ```java
> if (message.getAdvice() == null ||
"none".equals(message.getAdvice().get("reconnect")))
> ```
>
> The new code only handshakes when reconnect is non-null and not `"retry"`:
>
> ```java
> if (reconnectAdvice != null && !"retry".equals(reconnectAdvice))
> ```
>
> When the server sends no advice at all and the error is not a temporary
error, the old code would handshake but the new code does nothing. Could this
regress certain failure scenarios? If this change is intentional per the Bayeux
spec, it would be helpful to add a comment explaining the rationale.
>
> ### 5. Minor question — reconnect advice on successful connect
> The new code handles non-retry reconnect advice on _successful_ connect
messages. Has this scenario been observed in practice with Salesforce's CometD
server, or is it a defensive addition per the spec? Either way is fine, just
curious.
>
> Overall this is a solid improvement to the streaming subscription
resilience. Addressing the above (especially items 2–4) would make it ready to
merge. Thank you for the contribution!
>
> _Claude Code on behalf of Guillaume Nodet_
#1 (Missing JIRA ticket):
We can use this one https://issues.apache.org/jira/browse/CAMEL-23391
#2 (No tests):
The reconnect advice logic runs inside private CometD callbacks that require
heavy mocking of BayeuxClient, worker pool, and the component. The existing
tests in SubscriptionHelperTest follow the same pattern — only
static/package-private methods are covered. Happy to add tests if you can
suggest a preferred approach for testing these listeners in a seprate PR.
#3 (Incomplete lock coverage):
channelsToSubscribe is a ConcurrentHashMap.newKeySet() — individual
operations like add() and remove() are already atomic and thread-safe. The
channelsLock is only needed for compound operations (clear() + addAll(),
snapshot + clear()) where atomicity of the pair matters. Wrapping add() in
subscribe() with channelsLock would risk deadlock: subscribe() is called under
the inherited lock, and the connection listener acquires channelsLock then
calls subscribe() which acquires lock — a lock ordering inversion. The remove()
calls are benign as you noted.
#4 (Null advice regression):
Good catch — fixed. Changed to reconnectAdvice == null ||
!"retry".equals(reconnectAdvice) which restores the original
handshake-on-null-advice behavior. Added a comment explaining the Bayeux spec
rationale.
#5 (Successful connect advice):
Yes there is a production issue where the listner stopped working after this.
30-Apr-2026 13:37:30.774 GMT DEBUG [Camel (camel-1) thread #90 -
SalesforceHttpClient]
[org.apache.camel.component.salesforce.internal.streaming.SubscriptionHelper]
[{}] - [CHANNEL:META_CONNECT]: {clientId=rc1evauvtfawssvkcmb7qse50uj,
advice={reconnect=none},
channel=/meta/connect, id=82, successful=true}
--
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]
