Skip to content

Commit

Permalink
Fix statistics writers sometimes uses the user's publisher (#2172)
Browse files Browse the repository at this point in the history 
* Refs #12390. Always get statistics publisher's impl

Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>

* Refs #12161. Fix test compilation with statistics enabled

Signed-off-by: Ricardo González <ricardo@richiware.dev>

* Refs #12391: Added regression test

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Refs #12391: Removed unnecesary statements

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Refs 12391. Update depth of statistics writer on test.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Refs 12391. Reduce number of expected heartbeat counts.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Co-authored-by: Javier Santiago <javiersantiago@eprosima.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
  • Loading branch information
@richiware @jsan-rt @MiguelCompany
3 people authored Nov 22, 2021
1 parent b508fec commit 35910ca
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 9 deletions.
14 changes: 14 additions & 0 deletions src/cpp/fastdds/domain/DomainParticipantImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,15 @@ Publisher* DomainParticipantImpl::create_publisher(
const PublisherQos& qos,
PublisherListener* listener,
const StatusMask& mask)
{
return create_publisher(qos, nullptr, listener, mask);
}

Publisher* DomainParticipantImpl::create_publisher(
const PublisherQos& qos,
PublisherImpl** impl,
PublisherListener* listener,
const StatusMask& mask)
{
if (!PublisherImpl::check_qos(qos))
{
Expand Down Expand Up @@ -507,6 +516,11 @@ Publisher* DomainParticipantImpl::create_publisher(
(void)ret_publisher_enable;
}

if (impl)
{
*impl = pubimpl;
}

return pub;
}

Expand Down
14 changes: 14 additions & 0 deletions src/cpp/fastdds/domain/DomainParticipantImpl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,20 @@ class DomainParticipantImpl
PublisherListener* listener = nullptr,
const StatusMask& mask = StatusMask::all());

/**
* Create a Publisher in this Participant.
* @param qos QoS of the Publisher.
* @param[out] impl Return a pointer to the created Publisher's implementation.
* @param listenerer Pointer to the listener.
* @param mask StatusMask
* @return Pointer to the created Publisher.
*/
Publisher* create_publisher(
const PublisherQos& qos,
PublisherImpl** impl,
PublisherListener* listener = nullptr,
const StatusMask& mask = StatusMask::all());

/**
* Create a Publisher in this Participant.
* @param profile_name Publisher profile name.
Expand Down
14 changes: 7 additions & 7 deletions src/cpp/statistics/fastdds/domain/DomainParticipantImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,12 +226,7 @@ efd::PublisherImpl* DomainParticipantImpl::create_publisher_impl(
const efd::PublisherQos& qos,
efd::PublisherListener* listener)
{
auto impl = new PublisherImpl(this, qos, listener, statistics_listener_);
if (nullptr == builtin_publisher_impl_)
{
builtin_publisher_impl_ = impl;
}
return impl;
return new PublisherImpl(this, qos, listener, statistics_listener_);
}

efd::SubscriberImpl* DomainParticipantImpl::create_subscriber_impl(
Expand All @@ -243,8 +238,13 @@ efd::SubscriberImpl* DomainParticipantImpl::create_subscriber_impl(

void DomainParticipantImpl::create_statistics_builtin_entities()
{
efd::PublisherImpl* builtin_publisher_impl = nullptr;

// Builtin publisher
builtin_publisher_ = create_publisher(efd::PUBLISHER_QOS_DEFAULT);
builtin_publisher_ = create_publisher(efd::PUBLISHER_QOS_DEFAULT, &builtin_publisher_impl);

builtin_publisher_impl_ = dynamic_cast<PublisherImpl*>(builtin_publisher_impl);
assert(nullptr != builtin_publisher_impl_);

// Enable statistics datawriters
// 1. Find fastdds_statistics PropertyPolicyQos
Expand Down
71 changes: 69 additions & 2 deletions test/blackbox/common/DDSBlackboxTestsStatistics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include <fastdds/dds/core/LoanableSequence.hpp>

#include <fastdds/dds/domain/DomainParticipant.hpp>
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/qos/DomainParticipantFactoryQos.hpp>
#include <fastdds/dds/subscriber/DataReader.hpp>
#include <fastdds/dds/subscriber/Subscriber.hpp>
#include <fastdds/dds/topic/TopicDescription.hpp>
Expand Down Expand Up @@ -59,8 +61,10 @@ static DataReader* enable_statistics(
Subscriber* subscriber,
const std::string& topic_name)
{
auto qos = statistics::dds::STATISTICS_DATAWRITER_QOS;
qos.history().depth = 10;
EXPECT_EQ(ReturnCode_t::RETCODE_OK, participant->enable_statistics_datawriter(
topic_name, statistics::dds::STATISTICS_DATAWRITER_QOS));
topic_name, qos));

auto topic_desc = participant->lookup_topicdescription(topic_name);
EXPECT_NE(nullptr, topic_desc);
Expand Down Expand Up @@ -168,7 +172,7 @@ TEST(DDSStatistics, simple_statistics_datareaders)
{"RTPS_SENT_TOPIC", statistics::RTPS_SENT_TOPIC, num_samples},
{"NETWORK_LATENCY_TOPIC", statistics::NETWORK_LATENCY_TOPIC, num_samples},
{"PUBLICATION_THROUGHPUT_TOPIC", statistics::PUBLICATION_THROUGHPUT_TOPIC, num_samples},
{"HEARTBEAT_COUNT_TOPIC", statistics::HEARTBEAT_COUNT_TOPIC, num_samples},
{"HEARTBEAT_COUNT_TOPIC", statistics::HEARTBEAT_COUNT_TOPIC, 1},
{"SAMPLE_DATAS_TOPIC", statistics::SAMPLE_DATAS_TOPIC, num_samples},
{"DISCOVERY_TOPIC", statistics::DISCOVERY_TOPIC, 1},
{"PDP_PACKETS_TOPIC", statistics::PDP_PACKETS_TOPIC, 1},
Expand Down Expand Up @@ -296,3 +300,66 @@ TEST(DDSStatistics, simple_statistics_second_writer)

#endif // FASTDDS_STATISTICS
}

// Regression test for #12390. Mostly a replication of test simple_statistics_second_writer but creating
// and additional publisher with partitions before the creation of the builtin publisher
TEST(DDSStatistics, statistics_with_partition_on_user)
{
#ifdef FASTDDS_STATISTICS
auto domain_id = GET_PID() % 100;

DomainParticipantQos p_qos = PARTICIPANT_QOS_DEFAULT;
DomainParticipantFactory* participant_factory = DomainParticipantFactory::get_instance();

// We disable the auto-enabling so the builtin entities do not get created.
DomainParticipantFactoryQos factory_qos;
participant_factory->get_qos(factory_qos);
factory_qos.entity_factory().autoenable_created_entities = false;
participant_factory->set_qos(factory_qos);

DomainParticipant* p1 = participant_factory->create_participant(domain_id, p_qos);
DomainParticipant* p2 = participant_factory->create_participant(domain_id, p_qos);

ASSERT_NE(nullptr, p1);
ASSERT_NE(nullptr, p2);

// Now we create a Publisher with a partition
PublisherQos pub_qos = PUBLISHER_QOS_DEFAULT;
pub_qos.partition().push_back("partition_a");
auto user_pub_1 = p1->create_publisher(pub_qos);

// We enable the participants
ASSERT_EQ(ReturnCode_t::RETCODE_OK, p1->enable());
ASSERT_EQ(ReturnCode_t::RETCODE_OK, p2->enable());

auto statistics_p1 = statistics::dds::DomainParticipant::narrow(p1);
auto statistics_p2 = statistics::dds::DomainParticipant::narrow(p2);

ASSERT_NE(nullptr, statistics_p1);
ASSERT_NE(nullptr, statistics_p2);

auto subscriber_p1 = p1->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
auto subscriber_p2 = p2->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
ASSERT_NE(nullptr, subscriber_p1);
ASSERT_NE(nullptr, subscriber_p2);

auto physical_data_reader_1 = enable_statistics(statistics_p1, subscriber_p1, statistics::PHYSICAL_DATA_TOPIC);
auto physical_data_reader_2 = enable_statistics(statistics_p2, subscriber_p2, statistics::PHYSICAL_DATA_TOPIC);
ASSERT_NE(nullptr, physical_data_reader_1);
ASSERT_NE(nullptr, physical_data_reader_2);

wait_statistics(physical_data_reader_1, 2, "PHYSICAL_DATA_TOPIC", 10u);
wait_statistics(physical_data_reader_2, 2, "PHYSICAL_DATA_TOPIC", 10u);

disable_statistics(statistics_p1, subscriber_p1, physical_data_reader_1, statistics::PHYSICAL_DATA_TOPIC);
disable_statistics(statistics_p2, subscriber_p2, physical_data_reader_2, statistics::PHYSICAL_DATA_TOPIC);

p1->delete_publisher(user_pub_1);
p2->delete_subscriber(subscriber_p2);
p1->delete_subscriber(subscriber_p1);

participant_factory->delete_participant(p2);
participant_factory->delete_participant(p1);

#endif // FASTDDS_STATISTICS
}
15 changes: 15 additions & 0 deletions test/mock/dds/DomainParticipantImpl/fastdds/domain/DomainParticipantImpl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,15 @@ class DomainParticipantImpl

Publisher* create_publisher(
const PublisherQos& qos,
PublisherListener* listener,
const StatusMask& mask)
{
return create_publisher(qos, nullptr, listener, mask);
}

Publisher* create_publisher(
const PublisherQos& qos,
PublisherImpl** impl,
PublisherListener* listener = nullptr,
const StatusMask& mask = StatusMask::all())
{
Expand All @@ -149,6 +158,12 @@ class DomainParticipantImpl
std::lock_guard<std::mutex> lock(mtx_pubs_);
publishers_[pub] = pubimpl;
pub->enable();

if (impl)
{
*impl = pubimpl;
}

return pub;
}

Expand Down

0 comments on commit 35910ca

Please sign in to comment.