You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/08/18 18:45:00 UTC
[GitHub] [trafficserver] rob05c opened a new pull request, #9046: Fix hosting.config reload
rob05c opened a new pull request, #9046:
URL: https://github.com/apache/trafficserver/pull/9046
This is an alternative to https://github.com/apache/trafficserver/pull/8664. That PR is fine in-itself and fixes a bug around the the atomic swap not actually reloading. But fixing that bug so that reloading hosting.config works exposes another deeper bug: the reloaded object has no synchronization around it.
Not only is the pointer itself never read with synchronization, even if it were, the object itself is never synchronized. So even if the pointer were atomic, the object reads could get different variables from different objects, when callers that need multiple variables really need them to come from a single object. Worse, if a request takes longer than the arbitrary delete timer, it could access freed memory.
This fixes the object to be synchronized, and all access behind a mutex. The reload swap and bad drive removal acquire a write lock, and all other accesses (none of which modify the object) acquire a read lock.
I really don't like adding a mutex to the request path. But the only other options I see are an incredibly complex and bug-prone lock-free solution, or not making it possible to reload hosting.config. As much as I dislike it, the mutex should never have contention except on reload or drive failure. So it's probably actually faster and fewer atomic instructions than even a lock-free solution.
Recommend backporting to 9.2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
traeak commented on PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#issuecomment-1226147329
[approve ci docs]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c merged pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
rob05c merged PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#issuecomment-1298320913
Cherry-picked to v9.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#discussion_r989274674
##########
iocore/cache/P_CacheHosting.h:
##########
@@ -152,23 +261,34 @@ class CacheHostTable
struct CacheHostTableConfig;
typedef int (CacheHostTableConfig::*CacheHostTabHandler)(int, void *);
struct CacheHostTableConfig : public Continuation {
- CacheHostTable **ppt;
- CacheHostTableConfig(CacheHostTable **appt) : Continuation(nullptr), ppt(appt)
+ CacheHostTableConfig(ReplaceablePtr<CacheHostTable> *appt) : Continuation(nullptr), ppt(appt)
{
SET_HANDLER((CacheHostTabHandler)&CacheHostTableConfig::mainEvent);
}
+ ~CacheHostTableConfig() {}
+
int
mainEvent(int event, Event *e)
{
(void)e;
(void)event;
- CacheHostTable *t = new CacheHostTable((*ppt)->cache, (*ppt)->type);
- CacheHostTable *old = (CacheHostTable *)ink_atomic_swap(&t, *ppt);
- new_Deleter(old, CACHE_MEM_FREE_TIMEOUT);
+
+ CacheType type = CACHE_HTTP_TYPE;
+ Cache *cache = nullptr;
+ {
+ ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(ppt);
+ type = hosttable->type;
+ cache = hosttable->cache;
+ }
+ ppt->reset(new CacheHostTable(cache, type));
+ // new_Deleter(old, CACHE_MEM_FREE_TIMEOUT);
Review Comment:
remove commented out code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a diff in pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#discussion_r997594325
##########
iocore/cache/P_CacheHosting.h:
##########
@@ -152,23 +261,34 @@ class CacheHostTable
struct CacheHostTableConfig;
typedef int (CacheHostTableConfig::*CacheHostTabHandler)(int, void *);
struct CacheHostTableConfig : public Continuation {
- CacheHostTable **ppt;
- CacheHostTableConfig(CacheHostTable **appt) : Continuation(nullptr), ppt(appt)
+ CacheHostTableConfig(ReplaceablePtr<CacheHostTable> *appt) : Continuation(nullptr), ppt(appt)
{
SET_HANDLER((CacheHostTabHandler)&CacheHostTableConfig::mainEvent);
}
+ ~CacheHostTableConfig() {}
+
int
mainEvent(int event, Event *e)
{
(void)e;
(void)event;
- CacheHostTable *t = new CacheHostTable((*ppt)->cache, (*ppt)->type);
- CacheHostTable *old = (CacheHostTable *)ink_atomic_swap(&t, *ppt);
- new_Deleter(old, CACHE_MEM_FREE_TIMEOUT);
+
+ CacheType type = CACHE_HTTP_TYPE;
+ Cache *cache = nullptr;
+ {
+ ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(ppt);
+ type = hosttable->type;
+ cache = hosttable->cache;
+ }
+ ppt->reset(new CacheHostTable(cache, type));
+ // new_Deleter(old, CACHE_MEM_FREE_TIMEOUT);
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
bryancall commented on PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#issuecomment-1230956501
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #9046: Fix hosting.config reload
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #9046:
URL: https://github.com/apache/trafficserver/pull/9046#issuecomment-1282745441
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org