cannot use QoS override parameters via `NodeParameter` callbacks

[fujitatomoya]

Bug report

Required Info:

• Operating System:
  • Ubuntu 22.04
• Installation type:
  • source build
• Version or commit hash:
  • https://github.com/ros2/ros2/commit/1f5bd8ed43beea199dabe48bc8023af3aba9806c
• DDS implementation:
  • Any
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

• register callback that creates the subscription with QosOverridingOptions via add_post_set_parameters_callback.
• update parameter successfully, the gets ParameterModifiedInCallbackException exception.

cd colcon_ws/src
git clone https://github.com/fujitatomoya/ros2_test_prover
cd colcon_ws
colcon build --symlink-install --packages-select prover_interfaces prover_rclcpp
ros2 run prover_rclcpp rclcpp_237

and, with another terminal

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param get /test_params switch
Boolean value is: False
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param set /test_params switch true
Set parameter successful
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param set /test_params switch false
Set parameter successful

this will generate the exception below,

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run prover_rclcpp rclcpp_2379
[INFO] [1701396695.327098876] [test_params]: Parameter switch changed: 1
[INFO] [1701396695.327206255] [test_params]: Creating Subscription
[ERROR] [1701396695.327343914] [test_params]: Caught Exception with cannot set or declare a parameter, or change the callback from within set callback
[INFO] [1701396699.343136018] [test_params]: Parameter switch changed: 0
[INFO] [1701396699.343206436] [test_params]: Destroying Subscription
^C[INFO] [1701396701.596623929] [rclcpp]: signal_handler(signum=2)

Expected behavior

No exception received.

Actual behavior

ParameterModifiedInCallbackException

Additional information

This behavior blocks https://github.com/ros2/rclcpp/pull/2378

[fujitatomoya]

this exception,

https://github.com/ros2/rclcpp/blob/26f6efa840cbac047a93dc21d055cfe9cde7270f/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp#L58-L80

comes from,

https://github.com/fujitatomoya/ros2_test_prover/blob/ab78461fa2fa9ce9f4cf5fe712a0fc307670028e/prover_rclcpp/src/rclcpp_2379.cpp#L52C1-L52C1

because it will call declare_qos_parameters, and this leads to call the parameter operations in the NodeParameter callbacks.

i think user expects that they can create publisher or subscription when the parameter is updated, i guess this is supported behavior. i am not sure if we really need ParameterMutationRecursionGuard...

@ros2/team what do you think?

[clalancette]

i think user expects that they can create publisher or subscription when the parameter is updated, i guess this is supported behavior. i am not sure if we really need ParameterMutationRecursionGuard...

We definitely needed it when I added it several years ago. There was a race that could occur and that I was hitting often in the code I was working on. But I'd have to go back and look at the PR that led to it to remember the situation. Maybe there is another way to fix the race, but we shouldn't remove it without understanding the consequences.

[fujitatomoya]

External configurability of QoS policies introduced after https://github.com/ros2/rclcpp/pull/781, user must be extremely careful with calling recursive function like this, otherwise it is really easy to loop forever. i think we should keep ParameterMutationRecursionGuard behavior, on the other hand i think https://github.com/ros2/rclcpp/pull/2378 can bring good improvement for rclcpp application. besides, creating subscription on parameter change can be common use case.

more flexible and soft-restriction to have the maximum recursive count? I am not sure if this is good idea. i would like to hear other thoughts.