Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

refactor: use shared_ptr to replace raw pointer #754

Merged
merged 5 commits into from
Feb 20, 2021
Merged

refactor: use shared_ptr to replace raw pointer #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
310ce42
refactor: use shared_ptr to replace raw pointer
zhangyifan27 Jan 26, 2021
810c37c
Update src/failure_detector/failure_detector.cpp
acelyc111 Feb 7, 2021
00fb1f5
fix
zhangyifan27 Feb 18, 2021
28312e2
fix
zhangyifan27 Feb 18, 2021
94e7f2b
fix virtual/override
zhangyifan27 Feb 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/dsn/dist/failure_detector/failure_detector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class failure_detector : public failure_detector_service,
{
public:
failure_detector();
virtual ~failure_detector() { unregister_ctrl_commands(); }
virtual ~failure_detector();

virtual void on_ping(const beacon_msg &beacon, ::dsn::rpc_replier<beacon_ack> &reply);

Expand All @@ -109,7 +109,7 @@ class failure_detector : public failure_detector_service,
uint32_t grace_seconds,
bool use_allow_list = false);

error_code stop();
void stop();

uint32_t get_lease_ms() const { return _lease_milliseconds; }
uint32_t get_grace_ms() const { return _grace_milliseconds; }
Expand Down
12 changes: 6 additions & 6 deletions include/dsn/dist/failure_detector_multimaster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,20 @@ class slave_failure_detector_with_multimaster : public dsn::fd::failure_detector
slave_failure_detector_with_multimaster(std::vector<::dsn::rpc_address> &meta_servers,
std::function<void()> &&master_disconnected_callback,
std::function<void()> &&master_connected_callback);
~slave_failure_detector_with_multimaster(void);
virtual ~slave_failure_detector_with_multimaster() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
virtual ~slave_failure_detector_with_multimaster() {}
virtual ~slave_failure_detector_with_multimaster() override {}

Copy link
Member

Choose a reason for hiding this comment

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

@neverchanje why need an override when it's already used virtual ?

Copy link
Contributor

Choose a reason for hiding this comment

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

override and virtual are for different purposes. virtual disclaims that this function might be inherited by subclasses. But override indicates that it overrides the parent, i.e dsn::fd::failure_detector.

Copy link
Contributor Author

@zhangyifan27 zhangyifan27 Feb 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/language/virtual: Even though destructors are not inherited, if a base class declares its destructor virtual, the derived destructor always overrides it.

https://en.cppreference.com/w/cpp/language/override: In a member function declaration or definition, override specifier ensures that the function is virtual and is overriding a virtual function from a base class.

Copy link
Member

Choose a reason for hiding this comment

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

Explicitly annotate overrides of virtual functions or virtual destructors with exactly one of an override or (less frequently) final specifier. Do not use virtual when declaring an override. Rationale: A function or destructor marked override or final that is not an override of a base class virtual function will not compile, and this helps catch common errors. The specifiers serve as documentation; if no specifier is present, the reader has to check all ancestors of the class in question to determine if the function or destructor is virtual or not.


virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context);
void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context) override;

// client side
virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes);
virtual void on_master_connected(::dsn::rpc_address node);
void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes) override;
void on_master_connected(::dsn::rpc_address node) override;

// server side
virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes)
void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) override
{
dassert(false, "invalid execution flow");
}
virtual void on_worker_connected(::dsn::rpc_address node)
void on_worker_connected(::dsn::rpc_address node) override
{
dassert(false, "invalid execution flow");
}
Expand Down
35 changes: 14 additions & 21 deletions src/failure_detector/failure_detector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ failure_detector::failure_detector()
_is_started = false;
}

failure_detector::~failure_detector()
{
stop();
unregister_ctrl_commands();
}

void failure_detector::register_ctrl_commands()
{
static std::once_flag flag;
Expand Down Expand Up @@ -100,30 +106,17 @@ error_code failure_detector::start(uint32_t check_interval_seconds,
return ERR_OK;
}

error_code failure_detector::stop()
void failure_detector::stop()
{
if (_is_started == false) {
return ERR_OK;
}

_is_started = false;

{
zauto_lock l(_lock);
for (auto &m : _masters) {
m.second.send_beacon_timer->cancel(true);
}
_tracker.cancel_outstanding_tasks();

_masters.clear();
_workers.clear();
}

if (_check_task != nullptr) {
_check_task->cancel(true);
_check_task = nullptr;
zauto_lock l(_lock);
if (!_is_started) {
return;
}

return ERR_OK;
_is_started = false;
_masters.clear();
_workers.clear();
}

void failure_detector::register_master(::dsn::rpc_address target)
Expand Down
5 changes: 0 additions & 5 deletions src/failure_detector/failure_detector_multimaster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,6 @@ slave_failure_detector_with_multimaster::slave_failure_detector_with_multimaster
_master_connected_callback = std::move(master_connected_callback);
}

slave_failure_detector_with_multimaster::~slave_failure_detector_with_multimaster(void)
{
_tracker.cancel_outstanding_tasks();
neverchanje marked this conversation as resolved.
Show resolved Hide resolved
}

void slave_failure_detector_with_multimaster::set_leader_for_test(rpc_address meta)
{
_meta_servers.group_address()->set_leader(meta);
Expand Down
13 changes: 1 addition & 12 deletions src/replica/replica_stub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ void replica_stub::initialize_start()
// init liveness monitor
dassert(NS_Disconnected == _state, "");
if (_options.fd_disabled == false) {
_failure_detector = new ::dsn::dist::slave_failure_detector_with_multimaster(
_failure_detector = std::make_shared<dsn::dist::slave_failure_detector_with_multimaster>(
_options.meta_servers,
[this]() { this->on_meta_server_disconnected(); },
[this]() { this->on_meta_server_connected(); });
Expand Down Expand Up @@ -2511,17 +2511,6 @@ void replica_stub::close()
_replicas.erase(_replicas.begin());
}
}

if (_failure_detector != nullptr) {
_failure_detector->stop();
delete _failure_detector;
_failure_detector = nullptr;
}

if (_log != nullptr) {
_log->close();
_log = nullptr;
}
}

std::string replica_stub::get_replica_dir(const char *app_type, gpid id, bool create_new)
Expand Down
2 changes: 1 addition & 1 deletion src/replica/replica_stub.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ class replica_stub : public serverlet<replica_stub>, public ref_counter
::dsn::rpc_address _primary_address;
char _primary_address_str[64];

::dsn::dist::slave_failure_detector_with_multimaster *_failure_detector;
std::shared_ptr<dsn::dist::slave_failure_detector_with_multimaster> _failure_detector;
mutable zlock _state_lock;
volatile replica_node_state _state;

Expand Down