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

Conversation

wmlynar
Copy link

@wmlynar wmlynar commented Feb 11, 2019

This pull request is intended to resolve issue #289

@jubeira
Copy link

jubeira commented Feb 11, 2019

Thanks for the contribution @wmlynar!
I'll review it as soon as I can.

Copy link

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

@wmlynar thanks again for the contribution, and sorry for the delay!

The code works well; I've left some comments regarding thread-safety and using base interfaces. If you could please address them, then I'll merge them into kinetic.

@@ -52,12 +53,15 @@
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.

@@ -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.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants