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.

Sign up for GitHub

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

Add comments to explain why no locking is necessary #330

Merged
merged 3 commits into from
Dec 5, 2024
Merged

Add comments to explain why no locking is necessary #330

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
870cc03
Add comments to explain why no locking is necessary
sramani-metro Dec 3, 2024
83a5671
Add additional points
sramani-metro Dec 5, 2024
cbe1186
Fix an inadvertent typo
sramani-metro Dec 5, 2024
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
38 changes: 37 additions & 1 deletion Monitor/Monitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,8 @@ POP_WARNING()
}
inline void Open(PluginHost::IShell* service, Core::JSON::ArrayType<Config::Entry>::Iterator& index)
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */

ASSERT((service != nullptr) && (_service == nullptr));

uint64_t baseTime = Core::Time::Now().Ticks();
Expand Down Expand Up @@ -730,6 +732,8 @@ POP_WARNING()
}
inline void Close()
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */

ASSERT(_service != nullptr);

_job.Revoke();
Expand All @@ -740,6 +744,8 @@ POP_WARNING()
}
void Activated (const string& callsign, PluginHost::IShell* service) override
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */

MonitorObjectContainer::iterator index(_monitor.find(callsign));

if (index != _monitor.end()) {
Expand All @@ -761,6 +767,8 @@ POP_WARNING()
}
void Deactivated (const string& callsign, PluginHost::IShell* service) override
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */

MonitorObjectContainer::iterator index(_monitor.find(callsign));

if (index != _monitor.end()) {
Expand Down Expand Up @@ -793,6 +801,7 @@ POP_WARNING()
}
void Snapshot(Core::JSON::ArrayType<Monitor::Data>& snapshot) const
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */
MonitorObjectContainer::const_iterator element(_monitor.cbegin());

// Go through the list of observations...
Expand All @@ -807,6 +816,7 @@ POP_WARNING()
}
bool Snapshot(const string& name, Monitor::MetaData& result, bool& operational) const
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */
bool found = false;


Expand Down Expand Up @@ -848,7 +858,7 @@ POP_WARNING()

void Snapshot(const string& callsign, Core::JSON::ArrayType<JsonData::Monitor::InfoInfo>* response) const
{

/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */
ASSERT(response != nullptr);

if (callsign.empty() == false) {
Expand All @@ -865,6 +875,7 @@ POP_WARNING()

bool Reset(const string& name, Monitor::MetaData& result, bool& operational)
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */
bool found = false;

MonitorObjectContainer::iterator index(_monitor.find(name));
Expand All @@ -881,6 +892,7 @@ POP_WARNING()

bool Reset(const string& name)
{
/* See comment in the Dispatch method on why no locking is here to protect the _monitor member */
bool found = false;

MonitorObjectContainer::iterator index(_monitor.find(name));
Expand All @@ -902,6 +914,30 @@ POP_WARNING()

void Dispatch()
{
/*
* The list _monitor in the MonitorObjects class is primarily modified in these scenarios:
*
* During the Open Call:
* The _monitor list is populated with objects when Open is called. New MonitorObject instances are constructed and added to _monitor using the emplace function.
*
* During the Close Call:
* The _monitor list is cleared in the Close method by invoking _monitor.clear().
*
* The list _monitor is accessed for read purposes (e.g., iterating over it, querying specific elements) in methods such as:
* Dispatch (evaluation logic)
* Snapshot (generating snapshots)
* Activated and Deactivated (modifying individual MonitorObject entries)
*
* It was made sure in the plugin Initialize/Deinitialze that the Activated/Deactivated notification
* cannot happen while Open/Close are called. It is important not to change the sequence of the code in Initialize/Deinitialize.
*
* Many of the member variables in MonitorObject (e.g., _nextSlot, _restartWindow, _active) are declared as std::atomic.
* These variables ensure thread-safe access without the need for explicit locking. Some of them are defined as const making them immutable.
*
* For the above reasons, It was determined that there is no need to use the adminLock to protect the MonitorObjects list even though the MonitorObjects are accessed from
* multiple threads.
* *
*/
uint64_t scheduledTime(Core::Time::Now().Ticks());
uint64_t nextSlot(static_cast<uint64_t>(~0));

Expand Down