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 2020/10/16 20:32:39 UTC
[GitHub] [trafficserver] duke8253 opened a new pull request #7281: Make s3_auth plugin auto reload the config at expiration time
duke8253 opened a new pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281
Parse expiration time and reload config at time out, expiration is assumed to be a timestamp in seconds.
```
expiration=1601354698
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r540434116
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -143,7 +149,31 @@ class ConfigCache
S3Config *get(const char *fname);
private:
- std::unordered_map<std::string, std::pair<S3Config *, int>> _cache;
+ struct _Data {
+ // This is incremented before and after cnf and load_time are set.
+ // Thus, an odd value indicates an update is in progress.
+ std::atomic<unsigned> update_status{0};
+
+ // A config from a file and the last time it was loaded.
+ // config should be written before load_time. That way,
+ // if config is read after load_time, the load time will
+ // never indicate config is fresh when it isn't.
+ std::atomic<S3Config *> config;
+ std::atomic<time_t> load_time;
+
+ _Data() {}
Review comment:
That would be equivalent, as well as letting the default constructor be defined implicitly. The only real usefulness for "= default" I've seen is to enable default move cons/assign when copy cons/assign have been deleted.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-769452662
@randall Can you please review this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r510214276
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -391,6 +451,15 @@ class S3Config
TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_REQUEST_HDR_HOOK, _cont);
}
+ void
+ schedule_conf_reload(long delay) const
+ {
+ TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET);
+ }
+
+ std::shared_mutex reload_mutex;
Review comment:
I don't see the need for a major rewrite of the config class to use PingPong. But the version of PingPong above initializes the instance of T to be written from the last one, which is not good in all cases, and not good with the config class. PingPong can be changed like this: https://godbolt.org/z/c4qhfW
I think with this version of PingPong, the only change to the config class (to use it for the T parameter) would be to move the two continuations outside of the class instance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r510247738
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -897,6 +980,74 @@ event_handler(TSCont cont, TSEvent event, void *edata)
return 0;
}
+// Calculate the delay to schedule the auto config reload continuation.
+// Since the tokens expire right at the expiration time, we try to schedule
+// a little ahead of it (up to 5 minutes with 1 minute increments), e.g.
+// if the time till expiration is 1000 seconds in the future, we will try
+// to schedule the reload to happen 700 seconds in the future; if the
+// expiration is 100 seconds in the future, we'll try to schedule the reload
+// 100 - 60 = 40 seconds in the future; if the expiration is 220 seconds
+// in the future, we'll still try to schedule the reload 220 - 60 * 3 = 40
+// seconds in the future.
+static long
+cal_reload_delay(long time_diff)
+{
+ if (time_diff > 300) {
+ return time_diff - 300;
+ } else {
+ for (int i = 0; i < 5; ++i) {
+ if (time_diff - 60 <= 0) {
+ break;
+ } else {
+ time_diff -= 60;
+ }
+ }
+ return time_diff;
+ }
+}
+
+int
+config_reloader(TSCont cont, TSEvent event, void *edata)
+{
+ TSDebug(PLUGIN_NAME, "reloading configs");
+ S3Config *s3 = static_cast<S3Config *>(TSContDataGet(cont));
+ S3Config *file_config = gConfCache.get(s3->conf_fname());
Review comment:
But isn't there a risk that two instances of the plugin, that both use the same config file, will try to reload it at the same time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r510221151
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -391,6 +451,15 @@ class S3Config
TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_REQUEST_HDR_HOOK, _cont);
}
+ void
+ schedule_conf_reload(long delay) const
+ {
+ TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET);
+ }
+
+ std::shared_mutex reload_mutex;
Review comment:
I think, when writing a new config, the first step would be to explicitly call the config destructor and then the constructor:
w.data().~S3Config();
::new(&w.data()) S3Config;
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509391676
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
Because the mutex doesn't guarantee there's no writer starvation .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-712334807
Can this behavior be disabled by default? I think most config loads are generally signaled by the operator through traffic_ctl
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall removed a comment on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bryancall removed a comment on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-769452392
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r508917812
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
Why is this busy waiting desirable, especially since it seems to be redundant with the mutex.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509391676
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
Because there's no guarantee there's no writer starvation with just the mutex.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r524326664
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -496,40 +598,56 @@ ConfigCache::get(const char *fname)
auto it = _cache.find(config_fname);
if (it != _cache.end()) {
- if (tv.tv_sec > (it->second.second + _ttl)) {
- // Update the cached configuration file.
- S3Config *s3 = new S3Config(false); // false == this config does not get the continuation
-
- TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str());
- it->second.second = tv.tv_sec;
- if (s3->parse_config(config_fname)) {
- it->second.first = s3;
+ if (tv.tv_sec > (it->second.load_time + _ttl)) {
+ unsigned update_status = it->second.update_status;
Review comment:
One thread could execute line 601 and see the condition to be true, but then get. preempted. A second thread could execute lines 601 through 630, then get preempted. The first thread could then resume and run through 621. When the second thread resumed, s3, containing the function return value would be pointing to a deleted S3Config instance. The fix is to move line 602, the reading of update_status to before line 601.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 edited a comment on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 edited a comment on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-712339032
If you don't have the `expiration` set in the config, or comment that line, it will not reload.
I think I'll change it to treat `expiration=0` as disabling auto reload.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509441658
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
https://stackoverflow.com/questions/57706952/does-stdshared-mutex-favor-writers-over-readers
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r510238269
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -897,6 +980,74 @@ event_handler(TSCont cont, TSEvent event, void *edata)
return 0;
}
+// Calculate the delay to schedule the auto config reload continuation.
+// Since the tokens expire right at the expiration time, we try to schedule
+// a little ahead of it (up to 5 minutes with 1 minute increments), e.g.
+// if the time till expiration is 1000 seconds in the future, we will try
+// to schedule the reload to happen 700 seconds in the future; if the
+// expiration is 100 seconds in the future, we'll try to schedule the reload
+// 100 - 60 = 40 seconds in the future; if the expiration is 220 seconds
+// in the future, we'll still try to schedule the reload 220 - 60 * 3 = 40
+// seconds in the future.
+static long
+cal_reload_delay(long time_diff)
+{
+ if (time_diff > 300) {
+ return time_diff - 300;
+ } else {
+ for (int i = 0; i < 5; ++i) {
+ if (time_diff - 60 <= 0) {
+ break;
+ } else {
+ time_diff -= 60;
+ }
+ }
+ return time_diff;
+ }
+}
+
+int
+config_reloader(TSCont cont, TSEvent event, void *edata)
+{
+ TSDebug(PLUGIN_NAME, "reloading configs");
+ S3Config *s3 = static_cast<S3Config *>(TSContDataGet(cont));
+ S3Config *file_config = gConfCache.get(s3->conf_fname());
Review comment:
Ah, OK, you are already avoid a disk file read while the config is locked. But I think you should consider switching to my implementation of the shared mutex, to avoid the busy waiting and generally simplify this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509403607
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -391,6 +451,15 @@ class S3Config
TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_REQUEST_HDR_HOOK, _cont);
}
+ void
+ schedule_conf_reload(long delay) const
+ {
+ TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET);
+ }
+
+ std::shared_mutex reload_mutex;
Review comment:
I like the idea, but to use this it seems that the whole `config` class needs to be re-written to work with it, feels like a clean up PR rather than a simple feature one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-712339032
If you don't have the `expiration` set in the config, or comment that line, it will not reload.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-769452254
[approve ci]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r540331394
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -143,7 +149,31 @@ class ConfigCache
S3Config *get(const char *fname);
private:
- std::unordered_map<std::string, std::pair<S3Config *, int>> _cache;
+ struct _Data {
+ // This is incremented before and after cnf and load_time are set.
+ // Thus, an odd value indicates an update is in progress.
+ std::atomic<unsigned> update_status{0};
+
+ // A config from a file and the last time it was loaded.
+ // config should be written before load_time. That way,
+ // if config is read after load_time, the load time will
+ // never indicate config is fresh when it isn't.
+ std::atomic<S3Config *> config;
+ std::atomic<time_t> load_time;
+
+ _Data() {}
Review comment:
For explicit default construction, this is what `= default;` is for.
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -143,7 +149,31 @@ class ConfigCache
S3Config *get(const char *fname);
private:
- std::unordered_map<std::string, std::pair<S3Config *, int>> _cache;
+ struct _Data {
Review comment:
Let's rename this to something more specific.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-784372253
Cherry-picked to v9.1.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall removed a comment on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bryancall removed a comment on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-769452254
[approve ci]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509433303
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
Ahh good point. In my implementation of "one writer multi reader" locks, I made sure there would be no writer starvation: https://github.com/ywkaras/trafficserver/blob/ecde23cf0368a47e4124612bbbbb75341d901463/src/tscore/OneWriterMultiReader.cc#L54 . But, looking at cppreference.com, it doesn't say that std::shared_mutex provided a guarantee against writer starvation. The PingPong approach would prevent writer starvation even if the std::shared_mutex implementation does not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 merged pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 merged pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-712325711
[approve ci]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r527264937
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -496,40 +598,56 @@ ConfigCache::get(const char *fname)
auto it = _cache.find(config_fname);
if (it != _cache.end()) {
- if (tv.tv_sec > (it->second.second + _ttl)) {
- // Update the cached configuration file.
- S3Config *s3 = new S3Config(false); // false == this config does not get the continuation
-
- TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str());
- it->second.second = tv.tv_sec;
- if (s3->parse_config(config_fname)) {
- it->second.first = s3;
+ unsigned update_status = it->second.update_status;
+ if (tv.tv_sec > (it->second.load_time + _ttl)) {
+ if (!(update_status & 1) && it->second.update_status.compare_exchange_strong(update_status, update_status + 1)) {
+ TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str());
+ s3 = new S3Config(false); // false == this config does not get the continuation
+
+ if (s3->parse_config(config_fname)) {
+ s3->set_conf_fname(fname);
+ } else {
+ // Failed the configuration parse... Set the cache response to nullptr
+ delete s3;
+ s3 = nullptr;
+ TSAssert(!"Configuration parsing / caching failed");
+ }
+
+ delete it->second.config;
+ it->second.config = s3;
+ it->second.load_time = tv.tv_sec;
+
+ // Update is complete.
+ ++it->second.update_status;
} else {
- // Failed the configuration parse... Set the cache response to nullptr
- delete s3;
- it->second.first = nullptr;
+ // This thread lost the race with another thread that is also reloading
+ // the config for this file. Wait for the other thread to finish reloading.
+ while (it->second.update_status & 1) {
+ // Hopefully yielding will sleep the thread at least until the next
+ // scheduler interrupt, preventing a busy wait.
+ std::this_thread::yield();
+ }
+ s3 = it->second.config;
Review comment:
Unfortunately, as much as I have already tortured poor Fei about this PR, I think there is still a remaining hypothetical race condition. Suppose a thread executes through line 630, and is preempted at some point while the value in s3 is still in use. While it is not running, suppose a second thread executes line 602, with a tv.tv_sec value such that the condition for the if statement is true. The second thread then continues on and executes line 616, before the first thread runs again. When the first thread is run again, it will do a use-after-free.
Since this seems extremely unlikely, I think we should go ahead and merge this PR, and then see how/if to fix this concern.
If we were using C++20, we could simply make ConfigCache::get() return a shared_ptr, and make the config field in _Data an atomic shared_ptr. How soon are we going to 20?
Barring that, we can make the shared_ptr approach work by replacing update_status with a mutex. This would have the added benefit of getting rid of a lot tricky logic I asked Fei to put in this function. (Note that this function never creates new map entries when the map is being accessed by multiple threads, so we only have to lock the mutex when accessing an existing map entry.). Since that could potential be a fair number of mutexes, but I think they would be low-contention, I suggest using a "slimmed-down" alternative mutex: https://github.com/apache/trafficserver/pull/7330
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#issuecomment-769452392
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r509433303
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -876,7 +953,13 @@ event_handler(TSCont cont, TSEvent event, void *edata)
switch (event) {
case TS_EVENT_HTTP_SEND_REQUEST_HDR:
if (request.initialize()) {
- status = request.authorize(s3);
+ while (true) {
Review comment:
Ahh good point. In my implementation of "one writer multi reader" locks, I made sure there would be no writer starvation: https://github.com/ywkaras/trafficserver/blob/ecde23cf0368a47e4124612bbbbb75341d901463/src/tscore/OneWriterMultiReader.cc#L54 . But, looking at cppreference.com, it doesn't say that std::shared_mutex provides a guarantee against writer starvation. The PingPong approach would prevent writer starvation even if the std::shared_mutex implementation does not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r524326664
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -496,40 +598,56 @@ ConfigCache::get(const char *fname)
auto it = _cache.find(config_fname);
if (it != _cache.end()) {
- if (tv.tv_sec > (it->second.second + _ttl)) {
- // Update the cached configuration file.
- S3Config *s3 = new S3Config(false); // false == this config does not get the continuation
-
- TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str());
- it->second.second = tv.tv_sec;
- if (s3->parse_config(config_fname)) {
- it->second.first = s3;
+ if (tv.tv_sec > (it->second.load_time + _ttl)) {
+ unsigned update_status = it->second.update_status;
Review comment:
One thread could execute line 601 and see the condition to be true, but then get. preempted. A second thread could execute lines 601 through 621, then get preempted. The first thread could then resume and run through 621. When the second thread resumed, s3, containing the function return value would be pointing to a deleted S3Config instance. The fix is to move line 602, the reading of update_status to before line 601.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7281: Make s3_auth plugin auto reload the config at expiration time
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7281:
URL: https://github.com/apache/trafficserver/pull/7281#discussion_r508918650
##########
File path: plugins/s3_auth/s3_auth.cc
##########
@@ -391,6 +451,15 @@ class S3Config
TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_REQUEST_HDR_HOOK, _cont);
}
+ void
+ schedule_conf_reload(long delay) const
+ {
+ TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET);
+ }
+
+ std::shared_mutex reload_mutex;
Review comment:
This locking strategy looks like it could block multiple event threads while a new config is being loaded. I suggest letting transactions use the old config while the new one is being loaded, using https://godbolt.org/z/3bq43z or something like it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org