Skip to content

fix hosting.config reload #8664

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

Closed
ezelkow1 wants to merge 1 commit into from
Closed

fix hosting.config reload #8664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions iocore/cache/P_CacheHosting.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class CacheHostTable
struct CacheHostTableConfig;
typedef int (CacheHostTableConfig::*CacheHostTabHandler)(int, void *);
struct CacheHostTableConfig : public Continuation {
CacheHostTable **ppt;
std::atomic<CacheHostTable **> ppt;
Copy link
Member

@rob05c rob05c Aug 15, 2022

Choose a reason for hiding this comment

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

I noticed this isn't used by anything external. It'd be nice to make it private, to enforce that, and make it easier for readers to know that

CacheHostTableConfig(CacheHostTable **appt) : Continuation(nullptr), ppt(appt)
{
SET_HANDLER((CacheHostTabHandler)&CacheHostTableConfig::mainEvent);
Expand All @@ -163,8 +163,9 @@ struct CacheHostTableConfig : public Continuation {
{
(void)e;
(void)event;
CacheHostTable *t = new CacheHostTable((*ppt)->cache, (*ppt)->type);
CacheHostTable *old = (CacheHostTable *)ink_atomic_swap(&t, *ppt);
CacheHostTable *t = new CacheHostTable((*ppt)->cache, (*ppt)->type);
auto old = *ppt;
(*ppt) = t;
Copy link
Member

@rob05c rob05c Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this needs to be a std::atomic<CacheHostTable *>* ppt or possibly std::atomic<std::atomic<CacheHostTable *>*> ppt;.

With the outer pointer-pointer being atomic, but not the inner, I believe in auto old = *ppt the dereference *ppt will be atomic, but not the assignment to old. Likewise, in (*ppt) = t; I believe the *ppt dereference will be atomic, but when it takes t and assigns it to the value-pointer-to-by ppt, I don't think that will be atomic.

new_Deleter(old, CACHE_MEM_FREE_TIMEOUT);
return EVENT_DONE;
}
Expand Down