Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] description gate refactoring #1476

Merged
merged 14 commits into from
Mar 22, 2024

Conversation

rex-schilasky
Copy link
Contributor

@rex-schilasky rex-schilasky commented Mar 18, 2024

Description

DescGate logic refactored:

  • SHM and UDP registrations collected in common SampleList
  • ApplyDescription/Type whole quality logic moved to desc_gate.cpp unit
  • eCAL Initialize flag ProcessReg removed (should be set always)

Related issues

Needs to be written.

@rex-schilasky rex-schilasky added cherry-pick-to-support/v5.12 Cherry pick these changes to support/v.5.12 cherry-pick-to-support/v5.13 Cherry pick these changes to support/v5.13 labels Mar 18, 2024
… receiver to collect information

descgate quality logic removed (will be obsolete with new id based descgate anyway)
@rex-schilasky rex-schilasky added cherry-pick-to-NONE Don't cherry-pick these changes and removed cherry-pick-to-support/v5.12 Cherry pick these changes to support/v.5.12 cherry-pick-to-support/v5.13 Cherry pick these changes to support/v5.13 labels Mar 20, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}
}
break;
case bct_unreg_service:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: switch has 3 consecutive identical branches [bugprone-branch-clone]

    case bct_unreg_service:
    ^
Additional context

ecal/core/src/ecal_descgate.cpp:196: last of these clones ends here

      break;
           ^

, const SDataTypeInformation& request_type_information_
, const SDataTypeInformation& response_type_information_)
{
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'service_method_tuple' of type 'std::tuple<std::string, std::string>' (aka 'tuple<basic_string, basic_string>') can be declared 'const' [misc-const-correctness]

Suggested change
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
std::tuple<std::string, std::string> const service_method_tuple = std::make_tuple(service_name_, method_name_);


Registration::Sample CRegistrationProvider::GetProcessRegisterSample()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'GetProcessRegisterSample' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/registration/ecal_registration_provider.h:74:

-     Registration::Sample GetProcessRegisterSample();
+     static Registration::Sample GetProcessRegisterSample();

}

bool CRegistrationProvider::RegisterClient()
Registration::Sample CRegistrationProvider::GetProcessUnregisterSample()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'GetProcessUnregisterSample' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/registration/ecal_registration_provider.h:75:

-     Registration::Sample GetProcessUnregisterSample();
+     static Registration::Sample GetProcessUnregisterSample();

@rex-schilasky rex-schilasky changed the title [core] possible data race in create/register/unregister logic [core] description gate refactoring Mar 21, 2024
… process all remaining samples, like unregistrations)
@rex-schilasky rex-schilasky marked this pull request as ready for review March 21, 2024 12:18
…ce in case of finalizing nodes with a high number of pub/sub/client/server entities
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

#include <ecal/ecal.h>
#include <gtest/gtest.h>
#include <util/frequency_calculator.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'util/frequency_calculator.h' file not found [clang-diagnostic-error]

#include <util/frequency_calculator.h>
         ^

@@ -193,3 +194,67 @@
}
}
}

TEST(core_cpp_util, ParallelGetTopics)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_util, ParallelGetTopics)
TEST(core_cpp_util /*unused*/, ParallelGetTopics /*unused*/)

std::string topic_name = "Test.ParallelUtilFunctions";
std::atomic<int> call_back_count = 0;

std::vector<std::unique_ptr<eCAL::CPublisher>> publishers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'publishers' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::unique_ptr<eCAL::CPublisher>> publishers;
std::vector<std::unique_ptr<eCAL::CPublisher>> publishers = 0;


std::vector<std::unique_ptr<eCAL::CPublisher>> publishers;
for (int pub_count = 0; pub_count < max_publisher_count; pub_count++) {
std::unique_ptr<eCAL::CPublisher> publisher = std::make_unique<eCAL::CPublisher>(topic_name + std::to_string(pub_count));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'publisher' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unique_ptr<eCAL::CPublisher> publisher = std::make_unique<eCAL::CPublisher>(topic_name + std::to_string(pub_count));
std::unique_ptr<eCAL::CPublisher> publisher = 0 = std::make_unique<eCAL::CPublisher>(topic_name + std::to_string(pub_count));


auto get_topics_from_ecal = [&]() {
size_t found_topics = 0;
std::vector<std::string> tmp_topic_names;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'tmp_topic_names' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::string> tmp_topic_names;
std::vector<std::string> tmp_topic_names = 0;

auto get_topics_from_ecal = [&]() {
size_t found_topics = 0;
std::vector<std::string> tmp_topic_names;
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'topics' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics;
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics = 0;

} while (found_topics != 0);
};

std::vector<std::thread> threads_container;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'threads_container' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::thread> threads_container;
std::vector<std::thread> threads_container = 0;

th.join();
}

std::vector<std::string> tmp_topic_names;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'tmp_topic_names' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::string> tmp_topic_names;
std::vector<std::string> tmp_topic_names = 0;

}

std::vector<std::string> tmp_topic_names;
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'topics' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics;
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics = 0;

@rex-schilasky rex-schilasky marked this pull request as draft March 21, 2024 16:52
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}
}
break;
case bct_reg_client:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: switch has 2 consecutive identical branches [bugprone-branch-clone]

    case bct_reg_client:
    ^
Additional context

ecal/core/src/ecal_descgate.cpp:206: last of these clones ends here

      break;
           ^


bool CDescGate::RemoveServiceDescription(const std::string& service_name_, const std::string& method_name_)
{
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'service_method_tuple' of type 'std::tuple<std::string, std::string>' (aka 'tuple<basic_string, basic_string>') can be declared 'const' [misc-const-correctness]

Suggested change
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
std::tuple<std::string, std::string> const service_method_tuple = std::make_tuple(service_name_, method_name_);

th.join();
}

std::vector<std::string> final_topic_names;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'final_topic_names' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::string> final_topic_names;
std::vector<std::string> final_topic_names = 0;

}

std::vector<std::string> final_topic_names;
std::unordered_map<std::string, eCAL::SDataTypeInformation> final_topics;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'final_topics' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unordered_map<std::string, eCAL::SDataTypeInformation> final_topics;
std::unordered_map<std::string, eCAL::SDataTypeInformation> final_topics = 0;

@rex-schilasky rex-schilasky marked this pull request as ready for review March 22, 2024 09:40
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

namespace
{
// TODO: remove me with new CDescGate
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& service_name_, const std::string& method_name_,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'service_name_' is unused [misc-unused-parameters]

Suggested change
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& service_name_, const std::string& method_name_,
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& method_name_,

ecal/core/src/ecal_descgate.cpp:217:

-         ApplyServiceDescription(sample_.service.sname, method.mname, request_type, response_type, GetServiceMethodQuality(sample_.service.sname, method.mname, request_type, response_type));
+         ApplyServiceDescription(sample_.service.sname, method.mname, request_type, response_type, GetServiceMethodQuality(method.mname, request_type, response_type));

Comment on lines 33 to 34
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& service_name_, const std::string& method_name_,
const eCAL::SDataTypeInformation& request_info_,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'method_name_' is unused [misc-unused-parameters]

Suggested change
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& service_name_, const std::string& method_name_,
const eCAL::SDataTypeInformation& request_info_,
eCAL::CDescGate::QualityFlags GetServiceMethodQuality(const std::string& service_name_, const eCAL::SDataTypeInformation& request_info_,

ecal/core/src/ecal_descgate.cpp:217:

-         ApplyServiceDescription(sample_.service.sname, method.mname, request_type, response_type, GetServiceMethodQuality(sample_.service.sname, method.mname, request_type, response_type));
+         ApplyServiceDescription(sample_.service.sname, method.mname, request_type, response_type, GetServiceMethodQuality(sample_.service.sname, request_type, response_type));

return quality;
}

eCAL::CDescGate::QualityFlags GetPublisherQuality(const std::string& topic_name_, const eCAL::SDataTypeInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'topic_name_' is unused [misc-unused-parameters]

Suggested change
eCAL::CDescGate::QualityFlags GetPublisherQuality(const std::string& topic_name_, const eCAL::SDataTypeInformation& topic_info_)
eCAL::CDescGate::QualityFlags GetPublisherQuality(const eCAL::SDataTypeInformation& topic_info_)

ecal/core/src/ecal_descgate.cpp:250:

-       ApplyTopicDescription(sample_.topic.tname, sample_.topic.tdatatype, GetPublisherQuality(sample_.topic.tname, sample_.topic.tdatatype));
+       ApplyTopicDescription(sample_.topic.tname, sample_.topic.tdatatype, GetPublisherQuality(sample_.topic.tdatatype));

return quality;
}

eCAL::CDescGate::QualityFlags GetSubscriberQuality(const std::string& topic_name_, const eCAL::SDataTypeInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'topic_name_' is unused [misc-unused-parameters]

Suggested change
eCAL::CDescGate::QualityFlags GetSubscriberQuality(const std::string& topic_name_, const eCAL::SDataTypeInformation& topic_info_)
eCAL::CDescGate::QualityFlags GetSubscriberQuality(const eCAL::SDataTypeInformation& topic_info_)

ecal/core/src/ecal_descgate.cpp:256:

-       ApplyTopicDescription(sample_.topic.tname, sample_.topic.tdatatype, GetSubscriberQuality(sample_.topic.tname, sample_.topic.tdatatype));
+       ApplyTopicDescription(sample_.topic.tname, sample_.topic.tdatatype, GetSubscriberQuality(sample_.topic.tdatatype));


bool CDescGate::GetServiceTypeNames(const std::string& service_name_, const std::string& method_name_, std::string& req_type_name_, std::string& resp_type_name_)
{
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'service_method_tuple' of type 'std::tuple<std::string, std::string>' (aka 'tuple<basic_string, basic_string>') can be declared 'const' [misc-const-correctness]

Suggested change
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
std::tuple<std::string, std::string> const service_method_tuple = std::make_tuple(service_name_, method_name_);


bool CDescGate::GetServiceDescription(const std::string& service_name_, const std::string& method_name_, std::string& req_type_desc_, std::string& resp_type_desc_)
{
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'service_method_tuple' of type 'std::tuple<std::string, std::string>' (aka 'tuple<basic_string, basic_string>') can be declared 'const' [misc-const-correctness]

Suggested change
std::tuple<std::string, std::string> service_method_tuple = std::make_tuple(service_name_, method_name_);
std::tuple<std::string, std::string> const service_method_tuple = std::make_tuple(service_name_, method_name_);

}
}
break;
case bct_unreg_service:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: switch has 3 consecutive identical branches [bugprone-branch-clone]

    case bct_unreg_service:
    ^
Additional context

ecal/core/src/ecal_descgate.cpp:248: last of these clones ends here

      break;
           ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}
}
break;
case bct_unreg_service:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: switch has 3 consecutive identical branches [bugprone-branch-clone]

    case bct_unreg_service:
    ^
Additional context

ecal/core/src/ecal_descgate.cpp:241: last of these clones ends here

      break;
           ^

@@ -193,3 +194,72 @@ TEST(core_cpp_util, Freq_ResettableFrequencyCalculator)
}
}
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_util /*unused*/, ParallelGetTopics /*unused*/)


auto create_publishers = [&]() {
std::string topic_name = "Test.ParallelUtilFunctions";
std::atomic<int> call_back_count{ 0 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'publishers' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::atomic<int> call_back_count{ 0 };
std::vector<std::unique_ptr<eCAL::CPublisher>> publishers = 0;

std::string topic_name = "Test.ParallelUtilFunctions";
std::atomic<int> call_back_count{ 0 };

std::vector<std::unique_ptr<eCAL::CPublisher>> publishers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'publisher' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::unique_ptr<eCAL::CPublisher>> publishers;
std::unique_ptr<eCAL::CPublisher> publisher = 0 = std::make_unique<eCAL::CPublisher>(topic_name + std::to_string(pub_count));

std::this_thread::sleep_for(std::chrono::milliseconds(waiting_time_thread));
};

auto get_topics_from_ecal = [&]() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'tmp_topic_names' is not initialized [cppcoreguidelines-init-variables]

Suggested change
auto get_topics_from_ecal = [&]() {
std::vector<std::string> tmp_topic_names = 0;

};

auto get_topics_from_ecal = [&]() {
size_t found_topics = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'topics' is not initialized [cppcoreguidelines-init-variables]

Suggested change
size_t found_topics = 0;
std::unordered_map<std::string, eCAL::SDataTypeInformation> topics = 0;

std::cout << "Number of topics found by ecal: " << found_topics << "\n";
std::this_thread::sleep_for(std::chrono::milliseconds(500));
} while (found_topics != 0);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'threads_container' is not initialized [cppcoreguidelines-init-variables]

Suggested change
};
std::vector<std::thread> threads_container = 0;


for (auto& th : threads_container) {
th.join();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'final_topic_names' is not initialized [cppcoreguidelines-init-variables]

Suggested change
}
std::vector<std::string> final_topic_names = 0;

for (auto& th : threads_container) {
th.join();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'final_topics' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unordered_map<std::string, eCAL::SDataTypeInformation> final_topics = 0;

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner 👍

@@ -107,8 +106,7 @@ TEST(core_cpp_clientserver, GetServices)
EXPECT_EQ(resp_desc, "foo::resp_desc1-1");

// change attributes again (this will not overwrite the attributes anymore)
bool ret2 = server.AddDescription("foo::method1", "foo::req_type1-2", "foo::req_desc1-2", "foo::resp_type1-2", "foo::resp_desc1-2");
EXPECT_EQ(ret2, false);
server.AddDescription("foo::method1", "foo::req_type1-2", "foo::req_desc1-2", "foo::resp_type1-2", "foo::resp_desc1-2");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new behavior better. Either you are allowed to set attributes, or you are not. Being allowed to set attributes once only is strange, but later we'll move it to constructor and have the behavior anyways...

@@ -342,6 +317,7 @@ namespace eCAL

void CServiceServerImpl::Register(const bool force_)
{
if (!m_created) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this lead to other unwanted side-effects?

m_topics_map.erase(iter);
return(true);
// broadcast (updated) sample list over shm
SendSampleList2SHM();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need #ifdef here like above? (#if ECAL_CORE_REGISTRATION_SHM,...)

#endif

// send out sample list over udp
SendSampleList2UDP();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, here you need the #if ... ....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, here would be a great point for a new refactoring (not in this PR!) - Have a list of Registration Layers, over which you iterate - this saves you the #if in many places.

bool UnregisterClient(const std::string& client_name_, const std::string& client_id_, const Registration::Sample& ecal_sample_, bool force_);
using ApplySampleCallbackT = std::function<void(const Registration::Sample&)>;
void SetCustomApplySampleCallback(const std::string& customer_, const ApplySampleCallbackT& callback_);
void RemCustomApplySampleCallback(const std::string& customer_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think API-wise, something like this would be cleaner:

struct CallbackID
{
private: 
CallbackID();
int ID;
} 
// You can now also omit the customer string, but if you potentially want to know, keep it
CallbackID SetCustomApplySampleCallback(const std::string& customer_, const ApplySampleCallbackT& callback_);
void RemCustomApplySampleCallback(const CallbackID & customer_);

Pros:

  • You don't need a map with a string (which are more expensive for lookup than ints).
  • Also you cannot try to remove something that you have never added - harder to missuse

-> No need for this PR but in general...

@rex-schilasky rex-schilasky merged commit f1d8efe into master Mar 22, 2024
15 checks passed
@rex-schilasky rex-schilasky deleted the hotfix/register-unregister-data-race branch March 22, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants