feature/entity-unregistration #1086

rex-schilasky · 2023-04-27T07:23:16Z

Pull request type

Feature

What is the current behavior?
Destroyed entities (publisher, subscriber, client, server) do not active unregister from the monitoring layer. These entities are removed after a monitoring timeout that can be defined in the global configuration ecal.ini file ([monitoring]/timeout).

What is the new behavior?
New additional unregistration messages are fired by all ecal entities on destruction. Entities disappear immediately from the monitoring layer.

github-actions

clang-tidy made some suggestions

github-actions · 2023-04-27T07:49:00Z

ecal/core/src/pubsub/ecal_subscriber.cpp

@@ -137,8 +137,7 @@ namespace eCAL
    RemReceiveCallback();

    // first unregister data reader
-    if(g_subgate())               g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);
-    if(g_registration_provider()) g_registration_provider()->UnregisterTopic(m_datareader->GetTopicName(), m_datareader->GetTopicID());
+    if(g_subgate()) g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);


warning: implicit conversion 'eCAL::CSubGate *' -> bool [readability-implicit-bool-conversion]

Suggested change

if(g_subgate()) g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);

if(g_subgate() != nullptr) g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);

KerstinKeller

See my comment regarding the expmap.
The rest in general looks good, but it's a bit much to really tell if everythingis ok.

ecal/core/src/ecal_expmap.h

…ation

github-actions

clang-tidy made some suggestions

github-actions · 2023-04-28T08:58:51Z

ecal/core/src/ecal_registration_receiver.cpp

@@ -222,70 +216,42 @@ namespace eCAL
    case eCAL::pb::bct_reg_process:


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

case eCAL::pb::bct_reg_process: ^

Additional context

ecal/core/src/ecal_registration_receiver.cpp:220: last of these clones ends here

break; ^

github-actions · 2023-04-28T08:58:52Z

ecal/core/src/readwrite/ecal_reader_shm.cpp

-    size_t ret_size(0);
-    if (g_subgate()) ret_size = g_subgate()->ApplySample(topic_name_, topic_id_, buf_, len_, id_, clock_, time_, hash_, eCAL::pb::tl_ecal_shm);
-    return ret_size;
+    if (g_subgate())


warning: implicit conversion 'eCAL::CSubGate *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (g_subgate())

if (g_subgate() != nullptr)

github-actions · 2023-04-28T08:58:52Z

ecal/core/src/readwrite/ecal_reader_udp_mc.cpp

  {
-    if (!g_subgate()) return 0;
+    if (!g_subgate()) return false;


warning: implicit conversion 'eCAL::CSubGate *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (!g_subgate()) return false;

if (g_subgate() == nullptr) return false;

github-actions · 2023-04-28T08:58:52Z

ecal/core/src/readwrite/ecal_reader_udp_mc.h

@@ -40,7 +40,7 @@ namespace eCAL
  {
  public:
    bool HasSample(const std::string& sample_name_);
-    size_t ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_);
+    bool ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_);


warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change

bool ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_);

bool ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_) override;

…ct memory files that are read and written in the same process

github-actions

clang-tidy made some suggestions

github-actions · 2023-04-28T16:04:45Z

ecal/core/src/readwrite/ecal_reader_shm.cpp

-        std::string memfile_event = memfile_name + "_" + process_id;
-        MemFileDataCallbackT memfile_data_callback = std::bind(&CSHMReaderLayer::OnNewShmFileContent, this,
+        const std::string process_id = std::to_string(Process::GetProcessID());
+        const std::string memfile_event = memfile_name + "_" + process_id;


warning: string concatenation results in allocation of unnecessary temporary strings; consider using 'operator+=' or 'string::append()' instead [performance-inefficient-string-concatenation]

const std::string memfile_event = memfile_name + "_" + process_id; ^

github-actions · 2023-04-28T16:04:46Z

ecal/core/src/readwrite/ecal_writer_tcp.cpp

@@ -41,7 +40,7 @@

 namespace eCAL
 {
-  std::mutex                       CDataWriterTCP::g_tcp_writer_executor_mtx;
+  std::mutex                            CDataWriterTCP::g_tcp_writer_executor_mtx;


warning: variable 'g_tcp_writer_executor_mtx' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

std::mutex CDataWriterTCP::g_tcp_writer_executor_mtx; ^

friendly welcome and goodbye messages (Initialize & Finalize) logic fix in ecal.cpp and ecalc.cpp for GetVersion()

uname added to unregistration info

github-actions

clang-tidy made some suggestions

github-actions · 2023-05-01T08:51:47Z

ecal/core/src/service/ecal_servicegate.h

    void RefreshRegistrations();

  protected:
-    static std::atomic<bool>    m_created;
+    static std::atomic<bool> m_created;


warning: variable 'm_created' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static std::atomic<bool> m_created; ^

ecal/core/src/ecal.cpp

ecal/core/src/ecal_expmap.h

ecal/core/src/mon/ecal_monitoring_impl.cpp

ecal/core/src/mon/ecal_monitoring_impl.h

ecal/core/src/readwrite/ecal_reader.cpp

Because I can.

KerstinKeller · 2023-05-08T14:57:37Z

ecal/core/src/readwrite/ecal_writer_udp_mc.cpp

@@ -104,18 +102,18 @@ namespace eCAL
    // create new sample
    m_ecal_sample.Clear();
    m_ecal_sample.set_cmd_type(eCAL::pb::bct_set_sample);
-    auto ecal_sample_mutable_topic = m_ecal_sample.mutable_topic();
+    auto *ecal_sample_mutable_topic = m_ecal_sample.mutable_topic();


Does this not introduce a different behavior?

ecal/core/src/mon/ecal_monitoring_impl.cpp

KerstinKeller · 2023-09-12T08:33:01Z

ecal/core/src/readwrite/ecal_writer.cpp

@@ -654,10 +657,14 @@ namespace eCAL
  void CDataWriter::ApplyLocSubscription(const std::string& process_id_, const std::string& tid_, const std::string& ttype_, const std::string& tdesc_, const std::string& reader_par_)
  {
    Connect(tid_, ttype_, tdesc_);
+
+    // add key to local subscriber map
+    const std::string topic_key = process_id_ + tid_;


This introduces unwanted side-effects!

Introduces two new structs (SLocalSubscriptionInfo and SExternSubscriptionInfo) and uses them for applying / unapplying registrations. They are also stored in the ExpiredMap when topic are no longer visible on monitoring layer.

active unregistration for pub/sub,server,client,process

6468954

github-actions bot reviewed Apr 27, 2023

View reviewed changes

rex-schilasky marked this pull request as draft April 27, 2023 07:50

rex-schilasky added the enhancement New feature or request label Apr 27, 2023

rex-schilasky added this to the eCAL 5.12 milestone Apr 27, 2023

clang-tidy

db20fa6

rex-schilasky marked this pull request as ready for review April 27, 2023 09:41

KerstinKeller previously requested changes Apr 27, 2023

View reviewed changes

ecal/core/src/ecal_expmap.h Outdated Show resolved Hide resolved

publisher/subscriber local (expired) map entries removed by unregistr…

bb1d768

…ation

github-actions bot reviewed Apr 28, 2023

View reviewed changes

rex-schilasky added 3 commits April 28, 2023 11:10

clang-tidy

a1c99ee

intermediate fix for CDataWriterSHM::RemLocConnection to not Disconne…

4336625

…ct memory files that are read and written in the same process

logic fix

cce3093

hannemn self-requested a review April 28, 2023 13:23

clang-tidy all ecal/core/src/readwrite

412e8e7

github-actions bot reviewed Apr 28, 2023

View reviewed changes

rex-schilasky added 3 commits April 28, 2023 18:22

server/clients unregistration implemented

32a49dc

process unregistration

f116d16

friendly welcome and goodbye messages (Initialize & Finalize) logic fix in ecal.cpp and ecalc.cpp for GetVersion()

unused registration functions removed (service/client)

d443bc6

uname added to unregistration info

github-actions bot reviewed May 1, 2023

View reviewed changes

FlorianReimold reviewed May 4, 2023

View reviewed changes

ecal/core/src/ecal.cpp Outdated Show resolved Hide resolved

FlorianReimold reviewed May 4, 2023

View reviewed changes

ecal/core/src/ecal.cpp Outdated Show resolved Hide resolved

welcome and goodbye message removed again :-/

c007f53

hannemn requested changes May 9, 2023

View reviewed changes

ecal/core/src/readwrite/ecal_reader.cpp Outdated Show resolved Hide resolved

ecal/core/src/readwrite/ecal_reader.cpp Outdated Show resolved Hide resolved

rex-schilasky added 2 commits May 9, 2023 18:51

fixed a couple of issued based on the review of @hannemn

6732557

removed unintended comment

fcda52a

hannemn approved these changes May 10, 2023

View reviewed changes

rex-schilasky merged commit 1dd4624 into master May 10, 2023

rex-schilasky deleted the feature/entity-unregistration branch May 10, 2023 13:05

KerstinKeller reviewed Sep 12, 2023

View reviewed changes

KerstinKeller mentioned this pull request Sep 12, 2023

#1086 Breaks unregistration via monitoring expiration #1196

Closed

rex-schilasky mentioned this pull request Oct 17, 2023

broken pub/sub connection by dynamic recreation of a publisher or subscriber #1218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/entity-unregistration #1086

feature/entity-unregistration #1086

rex-schilasky commented Apr 27, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot Apr 27, 2023

KerstinKeller left a comment

github-actions bot left a comment

github-actions bot Apr 28, 2023

github-actions bot Apr 28, 2023

github-actions bot Apr 28, 2023

github-actions bot Apr 28, 2023

github-actions bot left a comment

github-actions bot Apr 28, 2023

github-actions bot Apr 28, 2023

github-actions bot left a comment

github-actions bot May 1, 2023

KerstinKeller May 8, 2023

KerstinKeller Sep 12, 2023

	if(g_subgate()) g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);
	if(g_subgate() != nullptr) g_subgate()->Unregister(m_datareader->GetTopicName(), m_datareader);

		@@ -222,70 +216,42 @@ namespace eCAL
		case eCAL::pb::bct_reg_process:

	if (!g_subgate()) return false;
	if (g_subgate() == nullptr) return false;

	bool ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_);
	bool ApplySample(const eCAL::pb::Sample& ecal_sample_, eCAL::pb::eTLayerType layer_) override;

feature/entity-unregistration #1086

feature/entity-unregistration #1086

Conversation

rex-schilasky commented Apr 27, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 27, 2023

Choose a reason for hiding this comment

KerstinKeller left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot Apr 28, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot May 1, 2023

Choose a reason for hiding this comment

KerstinKeller May 8, 2023

Choose a reason for hiding this comment

KerstinKeller Sep 12, 2023

Choose a reason for hiding this comment

rex-schilasky commented Apr 27, 2023 •

edited

Loading