Calling Dispose() on Subscriber raises an Exception

[TimoKettunen]

Issue

Calling Dispose for subscription breaks something in the framework, causing errors in log and preventing subsequent subscriptions.

Ways to reproduce:

Setup Unity project with plugins as instructed. With Unity code below (only essential code shown), start terminal and give command ros2 topic pub mytopic std_msgs/msg/String "data: 'hello'" When OnStartButtonClicked is called, messages are printed in log as expected. When OnStopButtonClicked is called, messages stop but in log is printed:

RuntimeError: subscription's implementation is invalid, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/subscription.c:462, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/wait.c:324
ROS2.Utils.CheckReturnEnum (System.Int32 ret) (at /workdir/ros2-for-unity/src/ros2cs/src/ros2cs/ros2cs_core/utils/Utils.cs:37)

If OnStartButtonClicked is called again, no messages are received anymore. This happens with standard and custom types in same way.

void Start(){ 
  m_ros2UnityComponent = transform.root.Find("RosNode").gameObject.GetComponent<ROS2UnityComponent>();
  ros2Node = m_ros2UnityComponent.CreateNode("ROS2UnityListenerNode"); 
}
void OnMessage(std_msgs.msg.String message){
  Debug.Log("Message received: " + message.Data);
}
void OnStartButtonClicked()
{ 
  if (m_subscriber == null){
    m_subscriber 
      = ros2Node.CreateSubscription<std_msgs.msg.String>("mytopic", OnMessage);
  }
}
void OnStopButtonClicked(){
  if (m_subscriber != null)
  {
    m_subscriber.Dispose();
    m_subscriber = null;
  }
}

Full stack trace

RuntimeError: subscription's implementation is invalid, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/subscription.c:462, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/wait.c:324
ROS2.Utils.CheckReturnEnum (System.Int32 ret) (at /workdir/ros2-for-unity/src/ros2cs/src/ros2cs/ros2cs_core/utils/Utils.cs:37)
ROS2.WaitSet.Wait (ROS2.rcl_context_t context, System.Collections.Generic.List`1[T] subscriptions, System.Double timeoutSec) (at /workdir/ros2-for-unity/src/ros2cs/src/ros2cs/ros2cs_core/WaitSet.cs:81)
ROS2.Ros2cs.SpinOnce (System.Collections.Generic.List`1[T] nodes, System.Double timeoutSec) (at /workdir/ros2-for-unity/src/ros2cs/src/ros2cs/ros2cs_core/Ros2cs.cs:233)
ROS2.ROS2UnityComponent.Tick () (at Assets/ros2_unity_core/ROS2/Scripts/ROS2UnityComponent.cs:129)
ROS2.ROS2UnityComponent.<FixedUpdate>b__16_0 () (at Assets/ros2_unity_core/ROS2/Scripts/ROS2UnityComponent.cs:140)
System.Threading.ThreadHelper.ThreadStart_Context (System.Object state) (at <fb001e01371b4adca20013e0ac763896>:0)
System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) (at <fb001e01371b4adca20013e0ac763896>:0)
System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) (at <fb001e01371b4adca20013e0ac763896>:0)
System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state) (at <fb001e01371b4adca20013e0ac763896>:0)
System.Threading.ThreadHelper.ThreadStart () (at <fb001e01371b4adca20013e0ac763896>:0)
UnityEngine.<>c:<RegisterUECatcher>b__0_0(Object, UnhandledExceptionEventArgs) (at /home/bokken/buildslave/unity/build/Runtime/Export/Scripting/UnhandledExceptionHandler.bindings.cs:46)

[adamdbrw]

It seems that there is a check missing in ros2cs in the Wait (checking whether subscription was disposed already, with a locked sync). This would cause a runtime error to propagate to R2FU primary component and it would be unloaded - meaning no new messages can arrive to (any) subscription.

I will investigate and fix this, thank you for raising the issue! Of course now you can also try it yourself - I would look in the WaitSet.cs file and add this check before line 80 (check whether subscription was disposed). It looks like we overlooked it during refactoring.

[adamdbrw]

Would you mind correcting the issue title to be "Calling Dispose() on Subscriber causes RuntimeError".

The fact that Unity handles MonoBehavior exceptions by unloading the behavior is standard - and this is what "breaks" R2FU since the main component (including the spinning) is no longer running.

The fix is quick but I would like to add a test for the case as well and we currently are preoccupied with ROS World.

@TimoKettunen you could wait for us to get some bandwidth (perhaps next week), but of course a PR is very welcome!

[TimoKettunen]

I fixed the title. We are busy too so we will wait until you can fix it next week or so.

[adamdbrw]

@TimoKettunen this is fixed should work for you now, take a look at the merge request by @kielczykowski-rai - he also added some tests for this case