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

Fix: In Reactor Add A Mutex To Protect HandlerList Internal Array #1229

Merged
merged 9 commits into from
Sep 6, 2024
47 changes: 25 additions & 22 deletions src/common/reactor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -331,27 +331,13 @@ namespace Pistache::Aio
HandlerList(const HandlerList& other) = delete;
HandlerList& operator=(const HandlerList& other) = delete;

HandlerList(HandlerList&& other) = default;
HandlerList& operator=(HandlerList&& other) = default;

HandlerList clone() const
{
HandlerList list;

for (size_t i = 0; i < index_; ++i)
{
list.handlers.at(i) = handlers.at(i)->clone();
}
list.index_ = index_;

return list;
}

Reactor::Key add(const std::shared_ptr<Handler>& handler)
{
if (index_ == MaxHandlers)
throw std::runtime_error("Maximum handlers reached");

GUARD_AND_DBG_LOG(handlers_arr_mutex_);

Reactor::Key key(index_);
handlers.at(index_++) = handler;

Expand All @@ -360,23 +346,37 @@ namespace Pistache::Aio

void removeAll()
{
GUARD_AND_DBG_LOG(handlers_arr_mutex_);

index_ = 0;
handlers.fill(NULL);
}

std::shared_ptr<Handler> operator[](size_t index) const
{
return handlers.at(index);
return handlers.at(index); // calls const "at"
}

std::shared_ptr<Handler> at(size_t index) const
{
std::shared_ptr<Handler> at(size_t index)
{ // It's const, except that the mutex is locked and unlocked
Copy link
Member

@kiplingw kiplingw Aug 20, 2024

Choose a reason for hiding this comment

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

@dgreatwood, if you want to guarantee to the caller that the method is logically const, you could try leaving it with a const qualifier, but setting the mutex as mutable.

if (index >= index_)
throw std::runtime_error("Attempting to retrieve invalid handler");

GUARD_AND_DBG_LOG(handlers_arr_mutex_);

return handlers.at(index);
}

std::shared_ptr<Handler> at(size_t index) const
{
// We cast away the "const" of "this" before calling the
// non-const version of "at". The non-const "at" is not
// declared const because it locks and unlocks a mutex; but it
// is const in that the class instance is in the same state
// after the call vs. before (mutex is unlocked again before
// non-const "at" exits) - so this is OK.
return(((HandlerList *)this)->at(index));
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using C-style casts. In this case I think const_cast would be more appropriate. Also, you could consider @kiplingw's suggestion; making a mutex mutable is a common pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Good eye @Tachi107. I missed that.

}

bool empty() const { return index_ == 0; }

size_t size() const { return index_; }
Expand Down Expand Up @@ -412,15 +412,18 @@ namespace Pistache::Aio
}

template <typename Func>
void forEachHandler(Func func) const
{
void forEachHandler(Func func)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here. Up to you, but you could optionally keep the const qualifier.

{ // It's const, except that the mutex is locked and unlocked
GUARD_AND_DBG_LOG(handlers_arr_mutex_);

for (size_t i = 0; i < index_; ++i)
func(handlers.at(i));
}

private:
std::array<std::shared_ptr<Handler>, MaxHandlers> handlers;
size_t index_;
std::mutex handlers_arr_mutex_;
};

HandlerList handlers_;
Expand Down
2 changes: 1 addition & 1 deletion version.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.4.1.20240804
0.4.2.20240820
Loading