From 1acd002a6e3f9c6359726fef168d187996af0acc Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Tue, 11 May 2021 14:37:50 +0000 Subject: [PATCH 1/6] Address assert on captive_action --- proxy/http/HttpCacheSM.cc | 2 ++ proxy/http/HttpSM.cc | 4 ++++ proxy/http/HttpSM.h | 7 +++++++ 3 files changed, 13 insertions(+) diff --git a/proxy/http/HttpCacheSM.cc b/proxy/http/HttpCacheSM.cc index 2bab3be01f4..fd22b24b781 100644 --- a/proxy/http/HttpCacheSM.cc +++ b/proxy/http/HttpCacheSM.cc @@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key) return ACTION_RESULT_DONE; } else { ink_assert(pending_action != nullptr || write_locked == true); + captive_action.cancelled = false; // Make sure not cancelled before we hand it out return &captive_action; } } @@ -375,6 +376,7 @@ HttpCacheSM::open_write(const HttpCacheKey *key, URL *url, HTTPHdr *request, Cac return ACTION_RESULT_DONE; } else { ink_assert(pending_action != nullptr); + captive_action.cancelled = false; // Make sure not cancelled before we hand it out return &captive_action; } } diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index ff510b4a84f..88526b1cace 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -2492,6 +2492,8 @@ HttpSM::state_cache_open_write(int event, void *data) { STATE_ENTER(&HttpSM : state_cache_open_write, event); + pending_action.try_clear(reinterpret_cast(data)); + // Make sure we are on the "right" thread if (ua_txn) { pending_action = ua_txn->adjust_thread(this, event, data); @@ -2612,6 +2614,8 @@ HttpSM::state_cache_open_read(int event, void *data) STATE_ENTER(&HttpSM::state_cache_open_read, event); milestones[TS_MILESTONE_CACHE_OPEN_READ_END] = Thread::get_hrtime(); + pending_action.try_clear(reinterpret_cast(data)); + ink_assert(server_entry == nullptr); ink_assert(t_state.cache_info.object_read == nullptr); diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h index 7513951b1df..f90ba5d94ca 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -196,6 +196,13 @@ class PendingAction { return pending_action; } + void + try_clear(Action *current_action) + { + if (current_action == pending_action) { + pending_action = nullptr; + } + } ~PendingAction() { if (pending_action) { From 684ea9318076d855e312f9715384cf0f7c80ba1e Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 12 May 2021 22:23:28 +0000 Subject: [PATCH 2/6] Reworked cancelled logic plus a new assert --- proxy/http/HttpCacheSM.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/proxy/http/HttpCacheSM.cc b/proxy/http/HttpCacheSM.cc index fd22b24b781..a7050f272b7 100644 --- a/proxy/http/HttpCacheSM.cc +++ b/proxy/http/HttpCacheSM.cc @@ -54,6 +54,7 @@ HttpCacheAction::cancel(Continuation *c) ink_assert(this->cancelled == 0); this->cancelled = 1; + ink_release_assert(false); if (sm->pending_action) { sm->pending_action->cancel(); } @@ -98,9 +99,12 @@ int HttpCacheSM::state_cache_open_read(int event, void *data) { STATE_ENTER(&HttpCacheSM::state_cache_open_read, event); - ink_assert(captive_action.cancelled == 0); pending_action = nullptr; + if (captive_action.cancelled == 1) { + return VC_EVENT_CONT; // SM gave up on us + } + switch (event) { case CACHE_EVENT_OPEN_READ: HTTP_INCREMENT_DYN_STAT(http_current_cache_connections_stat); @@ -158,8 +162,11 @@ int HttpCacheSM::state_cache_open_write(int event, void *data) { STATE_ENTER(&HttpCacheSM::state_cache_open_write, event); - ink_assert(captive_action.cancelled == 0); - pending_action = nullptr; + pending_action = nullptr; + + if (captive_action.cancelled == 1) { + return VC_EVENT_CONT; // SM gave up on us + } bool read_retry_on_write_fail = false; switch (event) { @@ -196,8 +203,6 @@ HttpCacheSM::state_cache_open_write(int event, void *data) if (read_retry_on_write_fail || open_write_tries <= master_sm->t_state.txn_conf->max_cache_open_write_retries) { // Retry open write; open_write_cb = false; - // reset captive_action since HttpSM cancelled it - captive_action.cancelled = 0; do_schedule_in(); } else { // The cache is hosed or full or something. @@ -266,8 +271,6 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key) } else { ink_assert(open_read_cb == false); } - // reset captive_action since HttpSM cancelled it during open read retry - captive_action.cancelled = 0; // Initialising read-while-write-inprogress flag this->readwhilewrite_inprogress = false; Action *action_handle = cacheProcessor.open_read(this, &key, this->read_request_hdr, this->http_params, this->read_pin_in_cache); From 23d825e275494134ee17cf2ab80fb13a390524a1 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 12 May 2021 23:34:38 +0000 Subject: [PATCH 3/6] One more attempt to clear the captive_action nicely --- proxy/http/HttpCacheSM.cc | 19 +++++++++++-------- proxy/http/HttpCacheSM.h | 1 + proxy/http/HttpSM.cc | 10 ++++++---- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/proxy/http/HttpCacheSM.cc b/proxy/http/HttpCacheSM.cc index a7050f272b7..108600b808d 100644 --- a/proxy/http/HttpCacheSM.cc +++ b/proxy/http/HttpCacheSM.cc @@ -115,10 +115,11 @@ HttpCacheSM::state_cache_open_read(int event, void *data) } open_read_cb = true; cache_read_vc = static_cast(data); - master_sm->handleEvent(event, data); + master_sm->handleEvent(event, &captive_action); break; case CACHE_EVENT_OPEN_READ_FAILED: + captive_action.err_code = reinterpret_cast(data); if ((intptr_t)data == -ECACHE_DOC_BUSY) { // Somebody else is writing the object if (open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries) { @@ -129,12 +130,12 @@ HttpCacheSM::state_cache_open_read(int event, void *data) // Give up; the update didn't finish in time // HttpSM will inform HttpTransact to 'proxy-only' open_read_cb = true; - master_sm->handleEvent(event, data); + master_sm->handleEvent(event, &captive_action); } } else { // Simple miss in the cache. open_read_cb = true; - master_sm->handleEvent(event, data); + master_sm->handleEvent(event, &captive_action); } break; @@ -175,7 +176,7 @@ HttpCacheSM::state_cache_open_write(int event, void *data) ink_assert(cache_write_vc == nullptr); cache_write_vc = static_cast(data); open_write_cb = true; - master_sm->handleEvent(event, data); + master_sm->handleEvent(event, &captive_action); break; case CACHE_EVENT_OPEN_WRITE_FAILED: @@ -211,8 +212,9 @@ HttpCacheSM::state_cache_open_write(int event, void *data) "[%" PRId64 "] [state_cache_open_write] cache open write failure %d. " "done retrying...", master_sm->sm_id, open_write_tries); - open_write_cb = true; - master_sm->handleEvent(event, data); + open_write_cb = true; + captive_action.err_code = reinterpret_cast(data); + master_sm->handleEvent(event, &captive_action); } break; @@ -223,7 +225,7 @@ HttpCacheSM::state_cache_open_write(int event, void *data) "falling back to read retry...", master_sm->sm_id, open_write_tries); open_read_cb = false; - master_sm->handleEvent(CACHE_EVENT_OPEN_READ, data); + master_sm->handleEvent(CACHE_EVENT_OPEN_READ, &captive_action); } else { Debug("http_cache", "[%" PRId64 "] [state_cache_open_write] cache open write failure %d. " @@ -359,7 +361,8 @@ HttpCacheSM::open_write(const HttpCacheKey *key, URL *url, HTTPHdr *request, Cac // Changed by YTS Team, yamsat Plugin if (open_write_tries > master_sm->redirection_tries && open_write_tries > master_sm->t_state.txn_conf->max_cache_open_write_retries) { - master_sm->handleEvent(CACHE_EVENT_OPEN_WRITE_FAILED, (void *)-ECACHE_DOC_BUSY); + captive_action.err_code = -ECACHE_DOC_BUSY; + master_sm->handleEvent(CACHE_EVENT_OPEN_WRITE_FAILED, &captive_action); return ACTION_RESULT_DONE; } diff --git a/proxy/http/HttpCacheSM.h b/proxy/http/HttpCacheSM.h index 2fc0c0ec2aa..d7297f4ab5b 100644 --- a/proxy/http/HttpCacheSM.h +++ b/proxy/http/HttpCacheSM.h @@ -50,6 +50,7 @@ struct HttpCacheAction : public Action { sm = sm_arg; }; HttpCacheSM *sm = nullptr; + int err_code = 0; }; class HttpCacheSM : public Continuation diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 88526b1cace..0674e3a8d7a 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -2492,8 +2492,6 @@ HttpSM::state_cache_open_write(int event, void *data) { STATE_ENTER(&HttpSM : state_cache_open_write, event); - pending_action.try_clear(reinterpret_cast(data)); - // Make sure we are on the "right" thread if (ua_txn) { pending_action = ua_txn->adjust_thread(this, event, data); @@ -2505,6 +2503,8 @@ HttpSM::state_cache_open_write(int event, void *data) ink_release_assert(vc && vc->thread == this_ethread()); } + pending_action.try_clear(reinterpret_cast(data)); + milestones[TS_MILESTONE_CACHE_OPEN_WRITE_END] = Thread::get_hrtime(); pending_action = nullptr; @@ -2616,6 +2616,8 @@ HttpSM::state_cache_open_read(int event, void *data) pending_action.try_clear(reinterpret_cast(data)); + HttpCacheAction *cache_action = reinterpret_cast(data); + ink_assert(server_entry == nullptr); ink_assert(t_state.cache_info.object_read == nullptr); @@ -2647,12 +2649,12 @@ HttpSM::state_cache_open_read(int event, void *data) pending_action = nullptr; SMDebug("http", "[%" PRId64 "] cache_open_read - CACHE_EVENT_OPEN_READ_FAILED with %s (%d)", sm_id, - InkStrerror(-(intptr_t)data), (int)(intptr_t)data); + InkStrerror(-cache_action->err_code), -cache_action->err_code); SMDebug("http", "[state_cache_open_read] open read failed."); // Inform HttpTransact somebody else is updating the document // HttpCacheSM already waited so transact should go ahead. - if ((intptr_t)data == -ECACHE_DOC_BUSY) { + if (cache_action->err_code == -ECACHE_DOC_BUSY) { t_state.cache_lookup_result = HttpTransact::CACHE_LOOKUP_DOC_BUSY; } else { t_state.cache_lookup_result = HttpTransact::CACHE_LOOKUP_MISS; From 254181b3a203b1df06aed282b44d3823387fd580 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 12 May 2021 23:45:56 +0000 Subject: [PATCH 4/6] Replace bool with ints --- proxy/http/HttpCacheSM.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proxy/http/HttpCacheSM.cc b/proxy/http/HttpCacheSM.cc index 108600b808d..1a6253786be 100644 --- a/proxy/http/HttpCacheSM.cc +++ b/proxy/http/HttpCacheSM.cc @@ -288,7 +288,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key) return ACTION_RESULT_DONE; } else { ink_assert(pending_action != nullptr || write_locked == true); - captive_action.cancelled = false; // Make sure not cancelled before we hand it out + captive_action.cancelled = 0; // Make sure not cancelled before we hand it out return &captive_action; } } @@ -382,7 +382,7 @@ HttpCacheSM::open_write(const HttpCacheKey *key, URL *url, HTTPHdr *request, Cac return ACTION_RESULT_DONE; } else { ink_assert(pending_action != nullptr); - captive_action.cancelled = false; // Make sure not cancelled before we hand it out + captive_action.cancelled = 0; // Make sure not cancelled before we hand it out return &captive_action; } } From 72a3652a9ef7d0fe16c00b79b3a5a42aedfe4564 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Thu, 13 May 2021 00:09:25 +0000 Subject: [PATCH 5/6] Tidy up for the fix --- proxy/http/HttpCacheSM.cc | 11 ++++++----- proxy/http/HttpCacheSM.h | 10 +++++++++- proxy/http/HttpSM.cc | 6 ++---- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/proxy/http/HttpCacheSM.cc b/proxy/http/HttpCacheSM.cc index 1a6253786be..025b8ee9e85 100644 --- a/proxy/http/HttpCacheSM.cc +++ b/proxy/http/HttpCacheSM.cc @@ -54,7 +54,6 @@ HttpCacheAction::cancel(Continuation *c) ink_assert(this->cancelled == 0); this->cancelled = 1; - ink_release_assert(false); if (sm->pending_action) { sm->pending_action->cancel(); } @@ -99,6 +98,7 @@ int HttpCacheSM::state_cache_open_read(int event, void *data) { STATE_ENTER(&HttpCacheSM::state_cache_open_read, event); + ink_assert(captive_action.cancelled == 0); pending_action = nullptr; if (captive_action.cancelled == 1) { @@ -119,7 +119,7 @@ HttpCacheSM::state_cache_open_read(int event, void *data) break; case CACHE_EVENT_OPEN_READ_FAILED: - captive_action.err_code = reinterpret_cast(data); + err_code = reinterpret_cast(data); if ((intptr_t)data == -ECACHE_DOC_BUSY) { // Somebody else is writing the object if (open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries) { @@ -163,6 +163,7 @@ int HttpCacheSM::state_cache_open_write(int event, void *data) { STATE_ENTER(&HttpCacheSM::state_cache_open_write, event); + ink_assert(captive_action.cancelled == 0); pending_action = nullptr; if (captive_action.cancelled == 1) { @@ -212,8 +213,8 @@ HttpCacheSM::state_cache_open_write(int event, void *data) "[%" PRId64 "] [state_cache_open_write] cache open write failure %d. " "done retrying...", master_sm->sm_id, open_write_tries); - open_write_cb = true; - captive_action.err_code = reinterpret_cast(data); + open_write_cb = true; + err_code = reinterpret_cast(data); master_sm->handleEvent(event, &captive_action); } break; @@ -361,7 +362,7 @@ HttpCacheSM::open_write(const HttpCacheKey *key, URL *url, HTTPHdr *request, Cac // Changed by YTS Team, yamsat Plugin if (open_write_tries > master_sm->redirection_tries && open_write_tries > master_sm->t_state.txn_conf->max_cache_open_write_retries) { - captive_action.err_code = -ECACHE_DOC_BUSY; + err_code = -ECACHE_DOC_BUSY; master_sm->handleEvent(CACHE_EVENT_OPEN_WRITE_FAILED, &captive_action); return ACTION_RESULT_DONE; } diff --git a/proxy/http/HttpCacheSM.h b/proxy/http/HttpCacheSM.h index d7297f4ab5b..9add57fa26a 100644 --- a/proxy/http/HttpCacheSM.h +++ b/proxy/http/HttpCacheSM.h @@ -50,7 +50,6 @@ struct HttpCacheAction : public Action { sm = sm_arg; }; HttpCacheSM *sm = nullptr; - int err_code = 0; }; class HttpCacheSM : public Continuation @@ -184,6 +183,12 @@ class HttpCacheSM : public Continuation abort_write(); } + inline int + get_last_error() const + { + return err_code; + } + private: void do_schedule_in(); Action *do_cache_open_read(const HttpCacheKey &); @@ -212,4 +217,7 @@ class HttpCacheSM : public Continuation // to keep track of multiple cache lookups int lookup_max_recursive = 0; int current_lookup_level = 0; + + // last error from the cache subsystem + int err_code = 0; }; diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 0674e3a8d7a..b655de357e6 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -2616,8 +2616,6 @@ HttpSM::state_cache_open_read(int event, void *data) pending_action.try_clear(reinterpret_cast(data)); - HttpCacheAction *cache_action = reinterpret_cast(data); - ink_assert(server_entry == nullptr); ink_assert(t_state.cache_info.object_read == nullptr); @@ -2649,12 +2647,12 @@ HttpSM::state_cache_open_read(int event, void *data) pending_action = nullptr; SMDebug("http", "[%" PRId64 "] cache_open_read - CACHE_EVENT_OPEN_READ_FAILED with %s (%d)", sm_id, - InkStrerror(-cache_action->err_code), -cache_action->err_code); + InkStrerror(-cache_sm.get_last_error()), -cache_sm.get_last_error()); SMDebug("http", "[state_cache_open_read] open read failed."); // Inform HttpTransact somebody else is updating the document // HttpCacheSM already waited so transact should go ahead. - if (cache_action->err_code == -ECACHE_DOC_BUSY) { + if (cache_sm.get_last_error() == -ECACHE_DOC_BUSY) { t_state.cache_lookup_result = HttpTransact::CACHE_LOOKUP_DOC_BUSY; } else { t_state.cache_lookup_result = HttpTransact::CACHE_LOOKUP_MISS; From ef8aa7700b8d828f15f8b546b4e3b8f92f45611c Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Thu, 13 May 2021 14:33:13 +0000 Subject: [PATCH 6/6] Update method name --- proxy/http/HttpSM.cc | 4 ++-- proxy/http/HttpSM.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index b655de357e6..b98155954bd 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -2503,7 +2503,7 @@ HttpSM::state_cache_open_write(int event, void *data) ink_release_assert(vc && vc->thread == this_ethread()); } - pending_action.try_clear(reinterpret_cast(data)); + pending_action.clear_if_action_is(reinterpret_cast(data)); milestones[TS_MILESTONE_CACHE_OPEN_WRITE_END] = Thread::get_hrtime(); pending_action = nullptr; @@ -2614,7 +2614,7 @@ HttpSM::state_cache_open_read(int event, void *data) STATE_ENTER(&HttpSM::state_cache_open_read, event); milestones[TS_MILESTONE_CACHE_OPEN_READ_END] = Thread::get_hrtime(); - pending_action.try_clear(reinterpret_cast(data)); + pending_action.clear_if_action_is(reinterpret_cast(data)); ink_assert(server_entry == nullptr); ink_assert(t_state.cache_info.object_read == nullptr); diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h index f90ba5d94ca..f5e94d385e9 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -197,7 +197,7 @@ class PendingAction return pending_action; } void - try_clear(Action *current_action) + clear_if_action_is(Action *current_action) { if (current_action == pending_action) { pending_action = nullptr;