Recording stopped prints only once.

[fujitatomoya]

part of #1529.

the follow up PR is required for #1529 (comment)

[fujitatomoya]

@MichaelOrlov minor fix, could you review?

[MichaelOrlov]

@fujitatomoya I was considering to put this check inside RecorderImpl::stop() method to cover other possible cases.
It is ok solution as intermitient fix.
However, will need to add protection with mutex for the in_recording_ in the future to avoid race conditions.

[fujitatomoya]

However, will need to add protection with mutex for the in_recording_ in the future to avoid race conditions.

rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp

Line 139 in 195e406

std::atomic<bool> in_recording_ = false;

it is atomic operation, so it should be protected?

[MichaelOrlov]

@fujitatomoya Changing a flag is atomic however, transitioning from one state to another (recording/not recording) is not.
There are possible situations when we are in the process of starting recording and calling the stop() method from another thread.
Let's do protection with mutex in a separate PR to be able to backport the current PR.

[fujitatomoya]

Ah, i see that resolves the problem too. i think we can close this PR.

[fujitatomoya]

@MichaelOrlov why did you reopen this one? do you want to merge this 1st and follow-up PR with mutex locks?

[MichaelOrlov]

@fujitatomoya

do you want to merge this 1st and follow-up PR with mutex locks?

Yes, exactly. Becasue if we will add a new mutex we will not be able to backport it to the Humble and Iron.

[fujitatomoya]

one thing i want to to confirm is, i believe #1529 (comment) is planned to be in RecorderImpl, which does not break the userspace. is that not acceptable?

[MichaelOrlov]

@fujitatomoya The problem is that we introduced RecorderImpl only on the Iron distro. Humble doesn't have it

rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp

Lines 113 to 131 in a296fb5

	Recorder::~Recorder()
	{
	keyboard_handler_->delete_key_press_callback(toggle_paused_key_callback_handle_);
	stop();
	}
	void Recorder::stop()
	{
	stop_discovery_ = true;
	if (discovery_future_.valid()) {
	auto status = discovery_future_.wait_for(2 * record_options_.topic_polling_interval);
	if (status != std::future_status::ready) {
	RCLCPP_ERROR_STREAM(
	get_logger(),
	"discovery_future_.wait_for(" << record_options_.topic_polling_interval.count() <<
	") return status: " << (status == std::future_status::timeout ? "timeout" : "deferred"));
	}
	}

[fujitatomoya]

The problem is that we introduced RecorderImpl only on the Iron distro. Humble doesn't have it

Ah 😅 I see. can you run CI for this? i am gonna follow up for #1529 (comment) with another PR.

[MichaelOrlov]

@fujitatomoya Yes, please go ahead and run CI. I 've approved this PR.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@MichaelOrlov looks good to go!

[MichaelOrlov]

https://github.com/Mergifyio backport iron humble

[mergify]

backport iron humble

✅ Backports have been created

• #1534 Recording stopped prints only once. (backport #1530) has been created for branch iron
• #1535 Recording stopped prints only once. (backport #1530) has been created for branch humble but encountered conflicts