Found incoherent method calls in rmw_create_subscription.

[Oscarchoi]

Bug report

In the following snippet of code, associate_reader(…)(line:104) and dissociate_writer(…)(line:114) do not correspond to each other.

rmw_fastrtps/rmw_fastrtps_cpp/src/rmw_subscription.cpp

Lines 101 to 124 in ccea7a3

	// Update graph
	std::lock_guard<std::mutex> guard(common_context->node_update_mutex);
	rmw_dds_common::msg::ParticipantEntitiesInfo msg =
	common_context->graph_cache.associate_reader(
	info->subscription_gid_, common_context->gid, node->name, node->namespace_);
	rmw_ret_t rmw_ret = rmw_fastrtps_shared_cpp::__rmw_publish(
	eprosima_fastrtps_identifier,
	common_context->pub,
	static_cast<void *>(&msg),
	nullptr);
	if (RMW_RET_OK != rmw_ret) {
	rmw_error_state_t error_state = *rmw_get_error_state();
	rmw_reset_error();
	static_cast<void>(common_context->graph_cache.dissociate_writer(
	info->subscription_gid_, common_context->gid, node->name, node->namespace_));
	rmw_ret = rmw_fastrtps_shared_cpp::destroy_subscription(
	eprosima_fastrtps_identifier, participant_info, subscription);
	if (RMW_RET_OK != rmw_ret) {
	RMW_SAFE_FWRITE_TO_STDERR(rmw_get_error_string().str);
	RMW_SAFE_FWRITE_TO_STDERR(" during '" RCUTILS_STRINGIFY(__function__) "' cleanup\n");
	rmw_reset_error();
	}
	rmw_set_error_state(error_state.message, error_state.file, error_state.line_number);
	return nullptr;

Considering that rmw_ds_common::GraphCache separates the gid storage of readers and writers, I think it has to dissociate the reader that was associated right above. In rmw_create_publisher(...), the corresponding counterpart of the codes, the associated writer is dissociated when publishing fails.

I think it should be modified as follows. Oscarchoi@e997378 If the existing code is not intended, is it okay to PR the above changes?
By the way, the test code assumes that rmw_create_subscription will always succeed, so this fallback code has no effect.

[fujitatomoya]

I think it should be modified as follows. Oscarchoi@e997378 If the existing code is not intended, is it okay to PR the above changes?

@MiguelCompany i think this is a bug, can you confirm?

[Oscarchoi]

I fixed formattings: Oscarchoi@7631039

[MiguelCompany]

@fujitatomoya I think @Oscarchoi is right, and the proposed changes seem correct.

[fujitatomoya]

@Oscarchoi as @MiguelCompany mentioned, this needs to be fixed as bug. Could you create PR with using your fix? I am happy to review 👍

[Oscarchoi]

Thank you both for confirmation! 😃
I'll create PR soon and notify you.

[clalancette]

This was fixed by #649 , so closing this out.