allthingssecurity commented on code in PR #28262:
URL: https://github.com/apache/camel/pull/28262#discussion_r4345214505
##########
components/camel-xmpp/src/main/java/org/apache/camel/component/xmpp/XmppComponent.java:
##########
@@ -92,14 +93,111 @@ protected void doStart() throws Exception {
@Override
protected void doStop() throws Exception {
- ServiceHelper.stopService(endpointCache.values());
- endpointCache.clear();
+ List<SharedConnection> remaining;
+ synchronized (connections) {
+ remaining = List.copyOf(connections.values());
+ connections.clear();
+ }
+ for (SharedConnection shared : remaining) {
+ shared.disconnect();
+ }
super.doStop();
}
- private String extractCacheKeyFromUri(String uri) throws
URISyntaxException {
- URI u = new URI(uri);
- return u.getScheme() + "://" + u.getHost() + u.getPort() +
u.getQuery();
+ /**
+ * Gets the connection of the endpoint's account for the given consumer or
producer, and creates it when the account
+ * has no connection yet or its connection is no longer connected. The
connection is shared with the other consumers
+ * and producers of the account, also of other endpoints; release it with
+ * {@link #releaseConnection(XmppEndpoint, Object)}.
+ */
+ XMPPTCPConnection acquireConnection(XmppEndpoint endpoint, Object user)
+ throws InterruptedException, IOException, SmackException,
XMPPException {
+ List<Object> key = endpoint.getConnectionKey();
+ SharedConnection shared;
+ synchronized (connections) {
+ shared = connections.computeIfAbsent(key, k -> new
SharedConnection());
+ shared.users.add(user);
+ }
+ boolean acquired = false;
+ shared.lock.lock();
+ try {
+ if (shared.connection == null || !shared.connection.isConnected())
{
+ shared.connection = endpoint.createConnection();
Review Comment:
Done in 81f6fe21a538. `acquireConnection` creates a connection only when the
account has none. Otherwise it connects and logs in the existing object under
the account's lock (`connectAndLogin`). `reconnect` uses the same helper now.
`AlreadyConnectedException` and `AlreadyLoggedInException` are tolerated. The
login uses Smack's `login()`, which logs in again with the user, password and
resource the connection used before (checked in the 4.4.8 jar: `login()` takes
`usedUsername`/`usedPassword`/`usedResource` before the configuration's
values). So it does not need the `login` helper of #28263, and the two PRs
still merge cleanly. New test
`connectionAcquiredDuringAnOutageIsTheConnectionOfTheAccount`: the server drops
the session, something of the account acquires during the outage, it gets the
same object, logged in again, and the producer that held it delivers again with
no `conflict`. With the old code it fails on `assertSame`. Since there is never
a second object now, `releaseCon
nection` disconnects the only one. It now also calls `disconnect()` on a
connection that is not connected, so Smack's `ReconnectionManager` stops
(`connectionClosed` sets its `done` flag); before, a dropped connection
released by its last user was reconnected later with nobody using it.
_Claude Code on behalf of allthingssecurity_
##########
components/camel-xmpp/src/main/java/org/apache/camel/component/xmpp/XmppConsumer.java:
##########
@@ -187,8 +187,16 @@ protected void doStop() throws Exception {
muc.leave();
muc = null;
}
- if (connection != null && connection.isConnected()) {
- connection.disconnect();
+ if (connection != null) {
+ // the connection is shared with the other consumers and producers
of the account: stop listening on it,
+ // and release it (it is closed when nobody uses it any more)
+ if (chatManager != null) {
+ chatManager.removeIncomingListener(this);
+ chatManager = null;
+ }
+ connection.removeSyncStanzaListener(this);
+ endpoint.releaseConnection(this);
Review Comment:
Done in 81f6fe21a538. In `XmppConsumer.doStop`, `muc.leave()` is in a `try`
whose `finally` removes the listeners (chat manager and stanza listener) and
releases the connection. The exception from `leave()` still propagates, and the
route stop logs it as before. `XmppGroupChatProducer.doStop` does the same
around `chat.leave()`. New tests
`consumerThatCannotLeaveItsRoomReleasesTheConnection` and
`groupChatProducerThatCannotLeaveItsRoomReleasesTheConnection` drop the server
connection, stop the route (so `leave()` throws `NotConnectedException`) and
check that the account has no users left (a package-private
`XmppComponent.connectionUsers`, for tests).
_Claude Code on behalf of allthingssecurity_
##########
components/camel-xmpp/src/main/java/org/apache/camel/component/xmpp/XmppEndpoint.java:
##########
@@ -177,18 +177,39 @@ protected String createEndpointUri() {
return "xmpp://" + host + ":" + port + "/" + getParticipant() +
"?serviceName=" + serviceName;
}
+ /**
+ * Gets the connection of this endpoint's account for the given consumer
or producer. The endpoints of one account
+ * share its connection (a server keeps one session per account and
resource); each endpoint keeps its own
+ * participant, room and other settings. Release it with {@link
#releaseConnection(Object)} when the consumer or
+ * producer stops: the connection is closed when no consumer or producer
of the account uses it any more.
+ */
+ public XMPPTCPConnection acquireConnection(Object user)
+ throws InterruptedException, IOException, SmackException,
XMPPException {
+ return getComponent().acquireConnection(this, user);
+ }
+
+ /**
+ * Releases the connection that the given consumer or producer acquired
with {@link #acquireConnection(Object)}.
+ */
+ public void releaseConnection(Object user) {
+ getComponent().releaseConnection(this, user);
+ }
+
+ /**
+ * The settings that make up the connection of an account: the endpoints
with the same settings share a connection.
+ */
+ List<Object> getConnectionKey() {
+ return Arrays.asList(host, port == 0 ? 5222 : port, serviceName, user,
password, resource, login, connectionConfig);
Review Comment:
Covered by the javadoc of `getConnectionKey` and the "Connections" section
of the component page (endpoints share a connection only when they refer to the
same `ConnectionConfiguration` instance). The new upgrade-guide entry says it
too. I am leaving this thread for you to resolve.
_Claude Code on behalf of allthingssecurity_
--
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]
