Skip to content
This repository has been archived by the owner on Aug 11, 2023. It is now read-only.

rosnode kill support (shut down the node) #291

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

rosnode kill support (shut down the node) #291

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion rosjava/src/main/java/org/ros/internal/node/DefaultNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public DefaultNode(NodeConfiguration nodeConfiguration, Collection<NodeListener>
nodeConfiguration.getTcpRosAdvertiseAddress(),
nodeConfiguration.getXmlRpcBindAddress(),
nodeConfiguration.getXmlRpcAdvertiseAddress(), masterClient, topicParticipantManager,
serviceManager, parameterManager, scheduledExecutorService);
serviceManager, parameterManager, scheduledExecutorService, this);
slaveServer.start();

NodeIdentifier nodeIdentifier = slaveServer.toNodeIdentifier();
Expand Down
16 changes: 15 additions & 1 deletion rosjava/src/main/java/org/ros/internal/node/server/SlaveServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.ros.address.AdvertiseAddress;
import org.ros.address.BindAddress;
import org.ros.internal.node.DefaultNode;
import org.ros.internal.node.client.MasterClient;
import org.ros.internal.node.parameter.ParameterManager;
import org.ros.internal.node.service.ServiceManager;
Expand Down Expand Up @@ -52,12 +53,15 @@ public class SlaveServer extends XmlRpcServer {
private final TopicParticipantManager topicParticipantManager;
private final ParameterManager parameterManager;
private final TcpRosServer tcpRosServer;
private final DefaultNode defaultNode;
Copy link

Choose a reason for hiding this comment

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

I'd suggest changing to Node instead of tying this to a particular implementation.
As we only need the shutdown method, we can use the base interface instead.

private boolean shutdownStarted;

public SlaveServer(GraphName nodeName, BindAddress tcpRosBindAddress,
AdvertiseAddress tcpRosAdvertiseAddress, BindAddress xmlRpcBindAddress,
AdvertiseAddress xmlRpcAdvertiseAddress, MasterClient master,
TopicParticipantManager topicParticipantManager, ServiceManager serviceManager,
ParameterManager parameterManager, ScheduledExecutorService executorService) {
ParameterManager parameterManager, ScheduledExecutorService executorService,
DefaultNode defaultNode) {
super(xmlRpcBindAddress, xmlRpcAdvertiseAddress);
this.nodeName = nodeName;
this.masterClient = master;
Expand All @@ -66,6 +70,7 @@ public SlaveServer(GraphName nodeName, BindAddress tcpRosBindAddress,
this.tcpRosServer =
new TcpRosServer(tcpRosBindAddress, tcpRosAdvertiseAddress, topicParticipantManager,
serviceManager, executorService);
this.defaultNode = defaultNode;
}

public AdvertiseAddress getTcpRosAdvertiseAddress() {
Expand All @@ -81,13 +86,22 @@ public void start() {
super.start(org.ros.internal.node.xmlrpc.SlaveXmlRpcEndpointImpl.class,
new SlaveXmlRpcEndpointImpl(this));
tcpRosServer.start();
shutdownStarted = false;
}

// TODO(damonkohler): This should also shut down the Node.
Copy link

Choose a reason for hiding this comment

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

This comment can be removed.

@Override
public void shutdown() {
// prevent recursive call of this method
if (shutdownStarted) {
return;
}
shutdownStarted = true;
Copy link

Choose a reason for hiding this comment

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

About thread safety:
if shutdown is called a second time after the if statement is processed but before shutdownStarted is set to true, it will be called twice.
To be completely sure, I'd suggest placing all the code in both shutdown and start in a synchronized block using a dummy object as the lock. E.g.:

  // In the constructor:
  shutdownLock = new Object();

  // Then:
  @Override
  public void shutdown() {
      synchronized(shutdownLock) {
          // current code goes here
      }
  }

And the same for start.

super.shutdown();
tcpRosServer.shutdown();
if (defaultNode != null) {
defaultNode.shutdown();
}
}

public List<Object> getBusStats(String callerId) {
Expand Down
2 changes: 1 addition & 1 deletion rosjava/src/test/java/org/ros/internal/node/MasterSlaveIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void setUp() {
new SlaveServer(GraphName.of("/foo"), BindAddress.newPrivate(),
AdvertiseAddress.newPrivate(), BindAddress.newPrivate(), AdvertiseAddress.newPrivate(),
masterClient, topicParticipantManager, serviceManager, parameterManager,
executorService);
executorService, null);
slaveServer.start();
slaveClient = new SlaveClient(GraphName.of("/bar"), slaveServer.getUri());
}
Expand Down