allthingssecurity commented on code in PR #28325:
URL: https://github.com/apache/camel/pull/28325#discussion_r4358016291


##########
components/camel-jgroups/src/main/java/org/apache/camel/component/jgroups/JGroupsEndpoint.java:
##########
@@ -227,15 +233,23 @@ private JChannel resolveChannel() throws Exception {
      * Connect shared channel, called by producer and consumer.
      */
     public void connect() throws Exception {
-        connectCount.incrementAndGet();
         LOG.trace("Connecting JGroups Channel {}", getEndpointUri());
+        if (!isOwnChannel()) {
+            ((JGroupsComponent) 
getComponent()).connectSharedChannel(resolvedChannel, clusterName);
+            return;
+        }
+        connectCount.incrementAndGet();
         resolvedChannel.connect(clusterName);

Review Comment:
   Applied in 5754d348428b: `connect()` counts after 
`resolvedChannel.connect(clusterName)` returns. New test 
`JGroupsEndpointOwnChannelTest.aProducerWhoseConnectFailedDoesNotKeepTheChannelConnected`:
 the endpoint's own channel fails its first connect (a `JChannel` subclass), so 
the producer fails to start; a consumer of the same endpoint then connects and 
stops, and the channel must be disconnected. With the count before the connect 
it stays connected (`the failed connect of the producer is not counted ==> 
expected: <false> but was: <true>`).
   
   The same commit also takes point 2: the own-channel 
`connect()`/`disconnect()` run under a `ReentrantLock` of the endpoint. 
`aConsumerThatStartsWhileTheProducerStopsKeepsTheChannelConnected` holds the 
producer's `disconnect()` until the consumer is either waiting on the lock or 
done, and fails without the lock (`connected for the consumer ==> expected: 
<true> but was: <false>`). To give the endpoint a channel in these tests, 
`resolveChannel()` is now package-private.
   
   _Claude Code on behalf of allthingssecurity_



##########
components/camel-jgroups/src/main/java/org/apache/camel/component/jgroups/JGroupsComponent.java:
##########
@@ -136,4 +150,61 @@ public void setAcceptAllObjects(boolean acceptAllObjects) {
         this.acceptAllObjects = acceptAllObjects;
     }
 
+    /**
+     * Connects a channel that several endpoints can use, for a producer or a 
consumer of one of them.
+     */
+    void connectSharedChannel(JChannel sharedChannel, String clusterName) 
throws Exception {
+        sharedChannelLock.lock();
+        try {
+            sharedChannel.connect(clusterName);
+            sharedChannelUsers.merge(sharedChannel, 1, Integer::sum);
+            sharedChannels.add(sharedChannel);
+        } finally {
+            sharedChannelLock.unlock();
+        }
+    }
+
+    /**
+     * Disconnects a channel that several endpoints can use, when its last 
producer or consumer stops.
+     */
+    void disconnectSharedChannel(JChannel sharedChannel) {
+        sharedChannelLock.lock();
+        try {
+            Integer users = sharedChannelUsers.get(sharedChannel);
+            if (users == null) {
+                return;
+            }
+            if (users > 1) {
+                sharedChannelUsers.put(sharedChannel, users - 1);
+            } else {
+                sharedChannelUsers.remove(sharedChannel);
+                sharedChannel.disconnect();
+            }
+        } finally {
+            sharedChannelLock.unlock();
+        }
+    }
+
+    @Override
+    protected void doStop() throws Exception {
+        super.doStop();
+        List<JChannel> channels;
+        sharedChannelLock.lock();
+        try {
+            channels = new ArrayList<>(sharedChannels);
+            sharedChannelUsers.clear();
+            sharedChannels.clear();
+        } finally {
+            sharedChannelLock.unlock();
+        }
+        // the channel of the component is used by all its endpoints, so it is 
closed with the component, as is a
+        // channel given to an endpoint
+        if (channel != null && !channels.contains(channel)) {
+            channels.add(channel);
+        }
+        for (JChannel sharedChannel : channels) {
+            sharedChannel.close();

Review Comment:
   Agreed, left as is in this PR: a channel the endpoints did not create is 
still closed at component stop, as on main, so `channel=#bean` cannot be used 
again after a stop/start of the CamelContext in place (a closed `JChannel` 
throws `channel is closed` on connect). Disconnecting rather than closing the 
channels Camel did not create changes what users see, so it needs its own 
upgrade-guide note; I drafted a follow-up JIRA for it.
   
   _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]

Reply via email to