fix hosting.config reload #8664
Conversation
|
suggestion from @SolidWallOfCode to take a look at the ipallow logic and see if we can do similar here |
|
Finally getting back to this, using the ipallow logic would take a very large rewrite from what I can tell. IMHO we should just do the simplest fix of swapping the values to the 'atomic_swap', the way it was meant to be used in the first place, to at least have this working. Then when we get around to a full hosting rewrite can look at doing it better then |
|
What we should really do is change it to a |
|
[approve ci] |
ezelkow1
force-pushed
the
hosting_stdatomic
branch
from
3af5541 to
9e31d6d
Compare
| CacheHostTable *old = (CacheHostTable *)ink_atomic_swap(&t, *ppt); | ||
| CacheHostTable *t = new CacheHostTable((*ppt)->cache, (*ppt)->type); | ||
| auto old = *ppt; | ||
| (*ppt) = t; |
There was a problem hiding this comment.
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.
| typedef int (CacheHostTableConfig::*CacheHostTabHandler)(int, void *); | ||
| struct CacheHostTableConfig : public Continuation { | ||
| CacheHostTable **ppt; | ||
| std::atomic<CacheHostTable **> ppt; |
There was a problem hiding this comment.
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
|
Is this pr still needed now that #9046 has been merged? |
nope, closing |
5 participants
When hosting.config reloads, currently the file is properly loaded and placed in a new hosting table. However when ats goes to place it in use in the current cache structure it's using an ink_atomic_swap(new_table, old_table). This function does not actually swap, it only does a set, so it effectively overwrites the new table with the old table, and then continues, never updating. This is why reloading with a new hosting.config never sees an update in use.
This PR changes that to get rid of the ink_atomic use and just use a std_atomic for the table, replaces the host table ptr, and deletes the old one
closes #7220