Repository navigation
CAMEL-25842: camel-xmpp - the consumer logs in and enters its room again after its connection was lost - #28263
Conversation
…ain after its connection was lost When the connection of XmppConsumer is lost, its connection monitor and Smack's ReconnectionManager (enabled by XmppComponent) both try to connect it again. The ReconnectionManager logs in after it connected, but gives up when the monitor connected first, and the monitor only called connection.connect(), which does not log in. An XMPP server delivers nothing to a stream that did not log in and bind a resource, so the consumer then reported a working connection and consumed nothing again until the route was restarted. And whoever logged in again, the consumer did not enter its room again, although the server removed it from the room when the connection was lost. The monitor now logs the connection in again when it is not authenticated (XmppEndpoint.login, split out of createConnection; the account is only created with the first connection; nothing is done when login=false). The consumer listens for every fresh login of its connection, by the monitor or by the ReconnectionManager, logs it and enters its room again in the monitor thread; the monitor tries again if entering fails. The monitor and the ReconnectionManager can connect and log in the same connection at the same time: Smack throws AlreadyConnectedException or AlreadyLoggedInException to the second one. The monitor now takes either as done by the other one (before, it logged a WARN with a stack trace for either, and skipped the login after AlreadyConnectedException). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Shashank,
I checked the Smack 4.4.8 side against the jar: ReconnectionManager returns without logging in when it finds the connection already connected, connect() and login() are synchronized and throw AlreadyConnectedException / AlreadyLoggedInException to the second caller, and MultiUserChat.join leaves first while Smack still thinks it is in the room. So the description holds, and entering the room again from the monitor thread is the right place. Smack's own MultiUserChatManager.setAutoJoinOnReconnect would not be enough here, as it enters again with the nickname only and drops the room password.
- Not blocking: the cleanup in
XmppConsumerSmackReconnectTestdoes not restore Smack's defaults.setDefaultFixedDelayalso sets the policy toFIXED_DELAY, so calling it aftersetDefaultReconnectionPolicy(RANDOM_INCREASING_DELAY)leaves later tests in the same JVM with a fixed 15 s delay. See inline. - Not blocking: the producers still only call
connect()after a lost connection (no login), and the group chat producer does not enter its room again either. You mention this follow-up on #28262; please file a JIRA for it so it is tracked. - FYI: this PR and #28262 merge without conflicts (both add the same
FakeXmppServer). Once both are in, the follow-up that movescheckConnectionontoendpoint.reconnect(connection)makes sense, so consumers and producers of an account reconnect under the same lock.
LGTM once the build jobs are green.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 25 (1 retried attempt) Recovered flaky tests on JDK 25 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 3 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (1 direct + 0 downstream), skip tests for 3 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (3)
All tested modules (35 modules, 5m 22s total)Total reactor time: 5m 22s
Top 20 slowest modules:
|
… the test cleanup (setDefaultFixedDelay first, as it also sets the policy) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks Claus. Pushed 53d929c:
camel-xmpp: 14 tests, 0 failures, 5 skipped (the tests that need an XMPP server container). Claude Code on behalf of allthingssecurity |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the quick update. The swapped ReconnectionManager calls in the cleanup look right, and the thread is addressed.
One thing still blocks the merge: CI. The run on 83d1734 (https://github.com/apache/camel/actions/runs/39697995006, build (25)) failed in camel-xmpp, and the failure comes from the new tests. They are the first camel-xmpp tests that load Smack on GitHub Actions; the other Smack tests need a container and are disabled there. All five failed on every rerun:
XmppConsumerConnectionRaceTest.connectionLoggedInMeanwhileByTheReconnectionManagerIsNotAFailure:77
Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.NoClassDefFoundError: org/jxmpp/util/cache/Cache
at org.jivesoftware.smack.SmackInitialization.loadSmackClass(SmackInitialization.java:210)
...
at org.jivesoftware.smack.AbstractXMPPConnection.<clinit>(AbstractXMPPConnection.java:175)
After that, XmppConsumerReconnectTest, XmppConsumerSmackReconnectTest and the second race test fail with Could not initialize class SmackInitialization / XMPPTCPConnectionConfiguration.
So on CI, jxmpp-util-cache does not end up on the test classpath, even though it does locally. My guess is version ranges plus the CI resolver: smack-core 4.4.8 asks for jxmpp-core/jxmpp-jid in the range [1.0.0,1.0.999], camel-xmpp pins jxmpp-jid to ${jxmpp-version} (1.1.0), and CI builds with mvnd and -Daether.dependencyCollector.impl=bf. Could you compare mvn dependency:tree -Daether.dependencyCollector.impl=bf in components/camel-xmpp with the default resolver? If jxmpp-util-cache drops out, declaring it explicitly next to jxmpp-jid (same ${jxmpp-version}) should fix the tests. It would also guard users whose builds resolve the same way.
The run for 53d929c is still queued. If it comes back green, please ignore this. #28262 adds the same FakeXmppServer and will probably hit the same problem.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
… on the test classpath
smack-core 4.4.8 asks for jxmpp-core and jxmpp-jid in the range [1.0.0,1.0.999],
while camel-xmpp pins jxmpp-jid to ${jxmpp-version} (1.1.0). The default resolver
then uses jxmpp 1.0.3 everywhere, but the CI build (mvnd) left jxmpp-util-cache off
the test classpath, so Smack failed to initialize (NoClassDefFoundError:
org/jxmpp/util/cache/Cache). Declaring it explicitly keeps it there for any resolver.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… on the test classpath
smack-core 4.4.8 asks for jxmpp-core and jxmpp-jid in the range [1.0.0,1.0.999],
while camel-xmpp pins jxmpp-jid to ${jxmpp-version} (1.1.0). The default resolver
then uses jxmpp 1.0.3 everywhere, but the CI build (mvnd) left jxmpp-util-cache off
the test classpath of apache#28263, so Smack failed to initialize (NoClassDefFoundError:
org/jxmpp/util/cache/Cache). This PR adds Smack tests too; declaring it explicitly
keeps it there for any resolver.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks Claus, good catch. Fixed in d59f575 (and the same change in #28262, 252c279). I compared the trees in components/camel-xmpp: locally the default resolver and The commit declares Separately, the 1.1.0 in Claude Code on behalf of allthingssecurity |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Shashank, declaring jxmpp-util-cache explicitly is the right answer to the CI failure. It is on the classpath whatever the resolver does. I compared the 1.0.3 and 1.1.0 jars, and the org.jxmpp.util.cache API is the same, so running it next to jxmpp-core 1.0.3 is fine. It merges cleanly with #28262, which adds the same lines.
Optional: as you noted, jxmpp-version (1.1.0) is never used for jxmpp-core and jxmpp-jid because of Smack's range. Please add that to the follow-up JIRA so it gets aligned.
Approving, to merge once CI is green.
Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Follow-up JIRA, with the Claude Code on behalf of allthingssecurity |
Description
CAMEL-25842
When the connection of
XmppConsumeris lost, two things try to connect it again: the consumer's connection monitor (everyconnectionPollDelayseconds) and Smack'sReconnectionManager, whichXmppComponent.doStartenables for every connection (its first attempt comes after a random delay of 2 to 14 seconds). TheReconnectionManagerlogs in after it connected, but it gives up when the monitor connected first, and the monitor only calledconnection.connect(), which does not log in with Smack 4.4. An XMPP server delivers nothing to a stream that did not log in and bind a resource, so when the monitor won the race the consumer reported a working connection and consumed nothing again (private chats and rooms) until the route was restarted. And whoever logged in again, the consumer did not enter its room again, although the server removes the occupant from a room when its connection ends: a room consumer never received a room message again after a lost connection.Changes:
XmppConsumer.checkConnection: when the connection is connected but not authenticated, log it in again (not whenlogin=false).XmppConsumer: aConnectionListenerwhoseauthenticated(connection, resumed)reacts to every fresh login of the connection (not to a resumed stream), by the monitor or by theReconnectionManager: it logs "Logged in again" at INFO and enters the room again (same nickname and room password) in the monitor thread, not in the thread that logged in. If entering fails, the monitor tries again at its next check. Nothing is logged or done when no login happened.XmppConsumer.checkConnection: the monitor and theReconnectionManagercan connect and log in the same connection at the same time. Smack 4.4.8 makesconnect()andlogin(...)synchronized and throwsAlreadyConnectedException/AlreadyLoggedInExceptionto the second caller (theReconnectionManageritself catches both). The monitor now takes either as done by the other one (DEBUG), and goes on with the login afterAlreadyConnectedException; before, it logged a WARN with a stack trace and, afterAlreadyConnectedException, skipped the login.XmppEndpoint.login: the login part ofcreateConnection, as the configured user or anonymously.createAccountstill only applies to the first connection.pom.xml:jxmpp-util-cacheis declared next tojxmpp-jid(same${jxmpp-version}). Smack needs it, and it comes in only through jxmpp-core, which Smack 4.4.8 asks for in the range[1.0.0,1.0.999]; the CI build left it off the test classpath (NoClassDefFoundError: org/jxmpp/util/cache/Cache, review).The producers' reconnect is not changed (a send on such a connection fails loudly there). No option changed, so no upgrade guide entry.
Tests:
XmppConsumerReconnectTest(2 tests) withFakeXmppServer, a small in-process XMPP server (plain TCP, SASL PLAIN and ANONYMOUS, resource binding, room presence) that, like a real server, delivers stanzas only to sessions that logged in and to the room's occupants. The real Smack client connects to it; the tests drop every connection on the server. WithconnectionPollDelay=1the consumer's monitor connects first. Without the change both fail (The consumers did not log in again ==> expected: <2> but was: <0>,The consumer did not enter the room again ==> expected: <1> but was: <0>); with the change both pass.XmppConsumerSmackReconnectTest: theReconnectionManagerwins the race (fixed delay of 1 s,connectionPollDelay=3600), connects and logs in. Without the change, and with only the monitor re-entering the room, it fails (The consumer did not enter the room again ==> expected: <1> but was: <0>); with the change it passes.XmppConsumerConnectionRaceTest(2 tests, no server): a connection whoseconnect()throwsAlreadyConnectedExceptionis still logged in by the monitor, and a login that throwsAlreadyLoggedInExceptionis no failure ofcheckConnection. Without that part of the change both fail (The monitor did not log the connection in ==> expected: <[[not-authenticated] (1)]> but was: <[]>,Unexpected exception thrown: ...AlreadyLoggedInException).-Daether.dependencyCollector.impl=bf).setDefaultFixedDelayfirst, since it also sets the policy toFIXED_DELAY; review).git merge-tree).Note: when Smack still thinks it is in the room,
MultiUserChat.joinfirst leaves it and waits for the server's answer; a server that does not answer the leave of a session that is gone delays the new entry by Smack's reply timeout (5 s by default).How it was found: variant analysis of CAMEL-25815 (server-side state, here the logged-in session and the room occupancy, lost on a reconnect and not created again), then the JUnit test.
XmppRobustConnectionTesthas been disabled since Smack 4.2 with the note that the reconnect does not seem to work; this is a likely cause, but I did not re-enable it (it needs the container).What was not run: a real XMPP server (the container tests); the full root build.
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built and tested camel-xmpp. No option and no component page changed. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) 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-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code