Concerns with BroadcastServer

[sauk2]

Summary

This issue details two distinct concerns related to the rmf_websocket::BroadcastServer - unsafe shutdown error and a request for improved logging.

Problem: Shutdown error (Bad file descriptor)

BroadcastServer shutdown causes the following errors,

[info] Error getting remote endpoint: system:9 (Bad file descriptor)
[info] asio async_shutdown error: system:9 (Bad file descriptor)
[info] handle_accept error: Operation canceled
[info] Stopping acceptance of new connections because the underlying transport is no longer listening.

This seems to indicate a race condition or improper cleanup of the ASIO resources. It always occurs immediately after _data->echo_server.stop_listening() is called within the stop() method

rmf_ros2/rmf_websocket/src/rmf_websocket/BroadcastServer.cpp

Lines 88 to 99 in a4a984a

	/// Stop Server
	void stop()
	{
	std::cout << "Stop BroadcastServer" << std::endl;
	if (_server_thread.joinable())
	{
	_data->echo_server.stop_listening();
	_data->echo_server.stop();
	// TODO: properly close all connections
	_server_thread.join();
	}
	}

Proposed fix

Ensure all server-level shutdown commands are posted to and executed on the io_service's thread (addressed in #457).

Feature Request: Replace stdout logging

Currently BroadcastServer uses std::cout for logging output which streams directly to standard output. I would like to propose replacing direct std::cout calls with ROS 2 logging. This would involve passing a node by modifying the BroadcastServer's constructor.

API/ABI compatibility concerns

This would likely constitute an API break for rmf_websocket as constructor for BroadcastServer would change. It might also have ABI implications that I am not aware of. An initial search of the all Open-RMF repos indicates that this class is only ever used in the following tests

• rmf_ros2/rmf_fleet_adapter/test/tasks/test_Loop.cpp

  Line 169 in a4a984a

  using WebsocketServer = rmf_websocket::BroadcastServer;

• rmf_ros2/rmf_fleet_adapter/test/tasks/test_Delivery.cpp

  Line 374 in a4a984a

  using WebsocketServer = rmf_websocket::BroadcastServer;

So the question to maintainers, is this break acceptable so that we can stream logs to ROS 2 logging instead of stdout?

[aaronchongth]

Thanks for putting this issue ticket together. I haven't looked into the fixes yet, however regarding migrating logging to ROS 2 logging, I think that's a great idea.

A new builder function can be introduced, alongside the existing make function, for example

static std::shared_ptr<BroadcastServer> make_with_logger(
    const int port,
    ApiMessageCallback callback,
    std::optional<ApiMsgType> msg_selection = std::nullopt,
    const rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface = nullptr);

The changes can be propagated in the implementation that ABI is not broken too. There will however be a need to carefully check if the logging interface is provided, before calling RCLCPP_INFO(node_logging_interface->get_logger(), ......)

[sauk2]

@aaronchongth Thanks for this suggestion! Should I also push this as part of #457 or would you prefer to keep it in a separate PR?

[aaronchongth]

I think it can be a separate PR, since it's a bug fix, it can even be backported. Per discussion during the community call, we foresee removal of the printouts to be rather trivial.

With your testing and application, if you've found logging messages at certain parts of the broadcast server to be useful, we'd appreciate it a lot if you could add it in too, alongside the new API and use of ROS 2 logging.

I'll merge #457 for now

[sauk2]

@aaronchongth Thanks for reviewing the improvements! I am also looking to backport these fixes into the jazzy branch so I have opened this PR #459. Let me if this works or if you would prefer a subset of these changes to be included in Jazzy.