Skip to content

Block the execution of cluster tasks on non existing nodes #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cpetzka wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Block the execution of cluster tasks on non existing nodes #55

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/java/org/jivesoftware/openfire/plugin/util/cluster/ClusterPacketRouter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.jivesoftware.openfire.plugin.util.cluster;

import org.jivesoftware.openfire.RemotePacketRouter;
import org.jivesoftware.openfire.cluster.ClusterManager;
import org.jivesoftware.util.cache.CacheFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -37,13 +38,16 @@ public class ClusterPacketRouter implements RemotePacketRouter {

private static Logger logger = LoggerFactory.getLogger(ClusterPacketRouter.class);

public boolean routePacket(byte[] nodeID, JID receipient, Packet packet) {
public boolean routePacket(byte[] nodeID, JID recipient, Packet packet) {
// Send the packet to the specified node and let the remote node deliver the packet to the recipient
try {
CacheFactory.doClusterTask(new RemotePacketExecution(receipient, packet), nodeID);
if (!ClusterManager.isClusterMember(nodeID)) {
return false;
}
CacheFactory.doClusterTask(new RemotePacketExecution(recipient, packet), nodeID);
return true;
} catch (IllegalStateException e) {
logger.warn("Error while routing packet to remote node: " + e);
} catch (Exception e) {
logger.warn("Error while routing packet to remote node",e);
Copy link
Member

@guusdk guusdk Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly do we reach a state where things are routed to non-existing nodes? If this hints at a bug in the Openfire implementation, then I think it warrants logging to 'error' rather than 'warn'.

Also, this is more nitpicking, but it would be useful to log the identifier of the non-existing node that this is being routed to.

Suggested change
logger.warn("Error while routing packet to remote node",e);
logger.error("Error while routing packet to remote node: {}", nodeID == null ? "(null)" : new String(nodeID, java.nio.charset.StandardCharsets.UTF_8), e);

Copy link
Contributor Author

@cpetzka cpetzka Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @guusdk,

the node id is already part of the exception message:

2020.11.03 17:07:45.965 ERROR [hz.openfire.cached.thread-4] (org.jivesoftware.openfire.spi.RoutingTableImpl:279) - Primary packet routing failed java.lang.IllegalArgumentException: Requested node 5ed5407e-e7fb-4b87-8ef0-9aff16dc07a0 not found in cluster at org.jivesoftware.openfire.plugin.util.cache.ClusteredCacheFactory.doClusterTask(ClusteredCacheFactory.java:397) ~[hazelcast-2.5.1-SNAPSHOT.jar!/:?] at org.jivesoftware.util.cache.CacheFactory.doClusterTask(CacheFactory.java:701) ~[xmppserver-4.6.0-SNAPSHOT.jar:4.6.0-SNAPSHOT]

Copy link
Contributor Author

@cpetzka cpetzka Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I see it would at least be logged on all other exceptions as well.

return false;
}
}
Expand Down