Exception when using parameterclient while having node spinning in other thread

[firesurfer]

See:
https://github.com/firesurfer/Segfault_demo
(I simply reused the demo)

It throws the following exception:

terminate called after throwing an instance of 'std::runtime_error'
  what():  Node has already been added to an executor.

when doing

 rclcpp::spin_some(node);

in another thread.

The exception doesn't get thrown every time but every second or third time the segfault_demo_client is run.

[wjwwood]

The spin family of functions which take a node as an argument are not thread safe or reentrant. Since you call spin_some(node) in your thread and spin_until_future_complete in your main thread:

https://github.com/firesurfer/Segfault_demo/blob/master/src/test.cpp#L30

I'm not surprised that you get the error, since both functions first add the node to an executor, spin, and then remove the node afterwards. So it is a race condition which results in an error when one of the spin functions tries to add the node while the other has already added it but not yet removed it.

Since you're spinning in your thread, there's no need to do spin_until_future_complete. The future you're waiting on will be set by the spin in the other thread, so you only need to wait on the future to be complete, see:

http://en.cppreference.com/w/cpp/thread/future/wait or http://en.cppreference.com/w/cpp/thread/future/wait_for or http://en.cppreference.com/w/cpp/thread/future/wait_until

[wjwwood]

Looking closer, either of the spin_until_future_complete calls could trigger a problem, not just the one on line 30. I put together a slightly modified version of your example that I think might help you out:

https://gist.github.com/wjwwood/5667039b95a682e3034b

Thanks for reporting the exception that was raised, but I think that it is expected behavior (even if it is not immediately obvious why). Hopefully my answers will help you out.

[firesurfer]

Thank you for your advice. I will change the corresponding lines of code in my application.
Do you have any plans to implement the spin functions threadsafe? I think this might become a source of programming mistakes/errors if ros2 will be used by a larger community.

[wjwwood]

The spin functions that do not take a node as an argument maybe thread safe already. But we will make it so or document that it is not as soon as we can. We need to do an api review of rclcpp at some point and at that time we'll more closely consider thread safety and memory allocation related properties.

[firesurfer]

Replacing the spin_until_future_complete functions did work for me, but just in case I use:

rclcpp::WallRate loop_rate(30);
    while (rclcpp::ok()) {

        rclcpp::spin_some(node);
        loop_rate.sleep();
    }

and not

rclcpp::spin(node);

as you proposed. In the second case I can't access the parameter service (The call fails after a timeout of 5 seconds)

[wjwwood]

Hmm, that doesn't sound right, I'll have to try that out locally.

[wjwwood]

@firesurfer I think what you've uncovered is another subtle bug (and race condition). I've open a ticket for it here: #209

[firesurfer]

I would really appreciate if it would be possible to implement the spin_until_future_complete function would be implemented threadsafe. By switching to future.wait calls everything works but I need an extra thread for spinning. This is kind of bad in case of a library like my ros2_components repo, because the user has to make sure he spins in an extra thread.

[wjwwood]

@firesurfer why do you need to spin in another thread if you're using spin_until_future_complete? You can use a spin in a thread OR spin_until_future_complete in your example and that would work. I think the only problem is when you do both... And I can't think of a reason to do that.

[firesurfer]

I meant this the other way round. If I use spin in a thread I can't use spin_until_future_complete. But I am building a library. I don't want to force the user to spin in an extra thread, which is needed if I use future.wait.

[wjwwood]

If you're spinning within a library (hidden from the user) and the user also spins, then I'd suggest you should use an executor in your library. Then there should be no problem. The rclcpp::spin* functions all operate on a global singleton executor, so you're implicitly using the same single threaded executor from different threads which is what is the issue. Simply do this in your library:

rclcpp::executors::SingeThreadedExecutor executor;
executor.add_node(my_node);
executor.spin();
// executor.spin_some();
// executor.spin_once();
// executor.spin_until_future_complete(future);

Then your node and your executor are completely isolated from the user of the library.

[firesurfer]

I finally found the time to deal with this problem again. I'm not sure if an extra executor and an extra node are the best solution in my case.

I created my library in a way that the user creates a node and passes it to the library like in this pseudocode:

node = Create_Node(...)

component1 = new Component(node,..)
component2 = new Component(node,..)

Every Component will create a parameter client and a publisher or an subscription based on its name and therefore needs the node.
I think it might become a bit difficult to create a node and an executor only used for all components a user creates. (And also the user will loose control about the node). Could you give me a hint how to deal with this scenario?

[jacquelinekay]

@wjwwood I don't think the spin functions actually use a global singleton executor. Each function initializes its own SingleThreadedExecutor on the stack at the beginning.

https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/executors.cpp

[wjwwood]

@jacquelinekay you're right, I'm not sure if I was just mistaken or if it used to be that way.

Either way, now I'm thinking that @firesurfer you may be adding the same node to multiple executors? That might be bad, probably would be bad, but I'm not sure since we've never done that.

[firesurfer]

In case of using a spin function and using spin_until_future_complete I think from what I've read in the ros 2 code that the same node is added to multiple executors. In case this is done in the same thread this won't be a problem but in case the spin function is called from another thread this will cause a crash.

How is it intended by design to interact with ros 2 publishers and services from multiple threads? Is there any article or design documentation available on how the system with executors and so on works (or is intended to work)?

[wjwwood]

How is it intended by design to interact with ros 2 publishers and services from multiple threads? Is there any article or design documentation available on how the system with executors and so on works (or is intended to work)?

If you want to spin in multiple threads, then I'd recommend using the MultiThreadedExecutor. Unfortunately there is no document to describe how to use these yet. I had not considered the idea of spinning from an arbitrary thread. I suppose a MultiThreadedExecutor could have thread-safe .spin_once() and/or .spin_until_future_complete(future) functions, which could be called from any thread. My gut reaction though is that you've designed the node incorrectly if you need to spin from arbitrary threads. We do, however, need to figure out how to do synchronous service calls from arbitrary threads and from within a spin callback. So I expect the executors will change a bit before all is said and done.

[firesurfer]

I don't think that I designed the node in a wrong way (but perhaps you could point out a better way).

I've got some code that does hardware interaction and that should be called as often as possible (was used in another framework before - I simply removed the interaction with the old framework). At first I added the spin_some or spin_once function in the main loop of the program that called the functions for hardware interaction. But in case there was a lot of communication to be done over ros 2 the spin* function took too long. So I moved it into another thread.

But I still need to fill in data into messages and publish them and do a parameter service call from time to time from the main loop where the hardware in polled from. So there has to be some interaction between threads. I could surely synchronize all interaction with ros 2 into the same thread, but I would expect from an framework that this kind of multithreaded interaction should be possible.

Another simple example I can imagine is simply having a blocking call that waits for keyboard inputs. In this case I think many users will come up with a solution that simply moves the call of the spin function in another thread, so it would still be possible to have interaction via ros 2 in the background.