From 4984fd3e01201079fc8aad771d94f9897a03a4d9 Mon Sep 17 00:00:00 2001 From: "Alan M. Carroll" Date: Tue, 12 Oct 2021 13:18:07 -0500 Subject: [PATCH] Promote class PendingAction from HttpSM.h for use in other classes. --- include/tscore/PendingAction.h | 129 +++++++++++++++++++++++++++++++++ proxy/http/HttpSM.cc | 38 +++++----- proxy/http/HttpSM.h | 49 +------------ 3 files changed, 149 insertions(+), 67 deletions(-) create mode 100644 include/tscore/PendingAction.h diff --git a/include/tscore/PendingAction.h b/include/tscore/PendingAction.h new file mode 100644 index 00000000000..c23bb9cb6b9 --- /dev/null +++ b/include/tscore/PendingAction.h @@ -0,0 +1,129 @@ +/** @file + +Container for a pending @c Action. + +@section license License + +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** Hold a pending @c Action. + * + * This is modeled on smart pointer classes. This class wraps a pointer to + * an @c Action (which is also the super type for @c Event). If cleared or + * re-assigned the current @c Action, if any, is canceled before the pointer is + * lost to avoid ghost actions triggering after the main continuation is gone. + * + * The class is aware of the special value @c ACTION_RESULT_DONE. If that is assigned + * the pending action will not be canceled or cleared. + */ +class PendingAction +{ + using self_type = PendingAction; + +public: + PendingAction() = default; + PendingAction(self_type const &) = delete; + ~PendingAction(); + self_type &operator=(self_type const &) = delete; + + /// Check if there is an action. + /// @return @c true if no action is present, @c false otherwise. + bool empty() const; + + /** Assign a new @a action. + * + * @param action The instance to store. + * @return @a this + * + * Any existing @c Action is canceled. + * Assignment is thread + */ + self_type &operator=(Action *action); + + /** Get the @c Continuation for the @c Action. + * + * @return A pointer to the continuation if there is an @c Action, @c nullptr if not. + */ + Continuation *get_continuation() const; + + /** Get the @c Action. + * + * @return A pointer to the @c Action is present, @c nullptr if not. + */ + Action *get() const; + + /** Clear the current @c Action if it is @a action. + * + * @param action @c Action to check. + * + * This clears the internal pointer without any side effect. it is used when the @c Action + * is handled and therefore should no longer be canceled. + */ + void clear_if_action_is(Action *action); + +private: + Action *pending_action = nullptr; +}; + +inline bool +PendingAction::empty() const +{ + return pending_action == nullptr; +} + +inline PendingAction & +PendingAction::operator=(Action *action) +{ + // Apparently HttpSM depends on not canceling the previous action if anew + // one completes immediately. Canceling the contained action in that case + // cause the HttpSm to permanently stall. + if (ACTION_RESULT_DONE != action) { + if (action != pending_action && pending_action != nullptr) { + pending_action->cancel(); + } + pending_action = action; + } + return *this; +} + +inline Continuation * +PendingAction::get_continuation() const +{ + return pending_action ? pending_action->continuation : nullptr; +} + +inline Action * +PendingAction::get() const +{ + return pending_action; +} + +inline PendingAction::~PendingAction() +{ + if (pending_action) { + pending_action->cancel(); + } +} + +inline void +PendingAction::clear_if_action_is(Action *action) +{ + if (action == pending_action) { + pending_action = nullptr; + } +} diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 1cc299f754f..aef9fc44ab9 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -1564,7 +1564,7 @@ plugins required to work with sni_routing. if (!lock.is_locked()) { api_timer = -Thread::get_hrtime_updated(); HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_api_callout); - ink_release_assert(pending_action.is_empty()); + ink_release_assert(pending_action.empty()); pending_action = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10)); return -1; } @@ -1889,7 +1889,7 @@ HttpSM::state_http_server_open(int event, void *data) SMDebug("http_track", "entered inside state_http_server_open"); STATE_ENTER(&HttpSM::state_http_server_open, event); ink_release_assert(event == EVENT_INTERVAL || event == NET_EVENT_OPEN || event == NET_EVENT_OPEN_FAILED || - pending_action.is_empty()); + pending_action.empty()); if (event != NET_EVENT_OPEN) { pending_action = nullptr; } @@ -1912,7 +1912,7 @@ HttpSM::state_http_server_open(int event, void *data) // Since the UnixNetVConnection::action_ or SocksEntry::action_ may be returned from netProcessor.connect_re, and the // SocksEntry::action_ will be copied into UnixNetVConnection::action_ before call back NET_EVENT_OPEN from SocksEntry::free(), // so we just compare the Continuation between pending_action and VC's action_. - ink_release_assert(pending_action.is_empty() || pending_action.get_continuation() == vc->get_action()->continuation); + ink_release_assert(pending_action.empty() || pending_action.get_continuation() == vc->get_action()->continuation); pending_action = nullptr; if (this->plugin_tunnel_type == HTTP_NO_PLUGIN_TUNNEL) { @@ -2426,7 +2426,7 @@ HttpSM::state_hostdb_lookup(int event, void *data) opt.host_res_style = ats_host_res_from(ua_txn->get_netvc()->get_local_addr()->sa_family, t_state.txn_conf->host_res_data.order); pending_action = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); - if (pending_action.is_empty()) { + if (pending_action.empty()) { call_transact_and_set_next_state(nullptr); } } break; @@ -2561,7 +2561,7 @@ HttpSM::state_cache_open_write(int event, void *data) // Make sure we are on the "right" thread if (ua_txn) { pending_action = ua_txn->adjust_thread(this, event, data); - if (!pending_action.is_empty()) { + if (!pending_action.empty()) { HTTP_INCREMENT_DYN_STAT(http_cache_open_write_adjust_thread_stat); return 0; // Go away if we reschedule } @@ -4288,7 +4288,7 @@ void HttpSM::do_hostdb_lookup() { ink_assert(t_state.dns_info.lookup_name != nullptr); - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); milestones[TS_MILESTONE_DNS_LOOKUP_BEGIN] = Thread::get_hrtime(); @@ -4306,7 +4306,7 @@ HttpSM::do_hostdb_lookup() opt.timeout = t_state.api_txn_dns_timeout_value; } pending_action = hostDBProcessor.getSRVbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_srv_info, d, 0, opt); - if (pending_action.is_empty()) { + if (pending_action.empty()) { char *host_name = t_state.dns_info.srv_lookup_success ? t_state.dns_info.srv_hostname : t_state.dns_info.lookup_name; opt.port = t_state.dns_info.srv_lookup_success ? t_state.dns_info.srv_port : @@ -4319,7 +4319,7 @@ HttpSM::do_hostdb_lookup() ats_host_res_from(ua_txn->get_netvc()->get_local_addr()->sa_family, t_state.txn_conf->host_res_data.order); pending_action = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); - if (pending_action.is_empty()) { + if (pending_action.empty()) { call_transact_and_set_next_state(nullptr); } } @@ -4352,7 +4352,7 @@ HttpSM::do_hostdb_lookup() pending_action = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, t_state.dns_info.lookup_name, 0, opt); - if (pending_action.is_empty()) { + if (pending_action.empty()) { call_transact_and_set_next_state(nullptr); } return; @@ -4365,7 +4365,7 @@ void HttpSM::do_hostdb_reverse_lookup() { ink_assert(t_state.dns_info.lookup_name != nullptr); - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); SMDebug("http_seq", "[HttpSM::do_hostdb_reverse_lookup] Doing reverse DNS Lookup"); @@ -4773,7 +4773,7 @@ HttpSM::do_cache_lookup_and_read() { // TODO decide whether to uncomment after finish testing redirect // ink_assert(server_txn == NULL); - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); HTTP_INCREMENT_DYN_STAT(http_cache_lookups_stat); @@ -4871,7 +4871,7 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in URL *o_url, *s_url; bool restore_client_request = false; - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); if (t_state.redirect_info.redirect_in_process) { o_url = &(t_state.redirect_info.original_url); @@ -5031,7 +5031,7 @@ HttpSM::do_http_server_open(bool raw) ink_assert(ua_entry != nullptr || t_state.req_flavor == HttpTransact::REQ_FLAVOR_SCHEDULED_UPDATE || t_state.req_flavor == HttpTransact::REQ_FLAVOR_REVPROXY); - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); ink_assert(t_state.current.server->dst_addr.network_order_port() != 0); char addrbuf[INET6_ADDRPORTSTRLEN]; @@ -5263,7 +5263,7 @@ HttpSM::do_http_server_open(bool raw) if (ccount > t_state.txn_conf->outbound_conntrack.max) { ct_state.release(); - ink_assert(pending_action.is_empty()); // in case of reschedule must not have already pending. + ink_assert(pending_action.empty()); // in case of reschedule must not have already pending. // If the queue is disabled, reschedule. if (t_state.http_config_param->global_outbound_conntrack.queue_size < 0) { @@ -7168,10 +7168,10 @@ HttpSM::kill_this() // state. This is because we are depending on the // callout to complete for the state machine to // get killed. - if (callout_state == HTTP_API_NO_CALLOUT && !pending_action.is_empty()) { + if (callout_state == HTTP_API_NO_CALLOUT && !pending_action.empty()) { pending_action = nullptr; - } else if (!pending_action.is_empty()) { - ink_assert(pending_action.is_empty()); + } else if (!pending_action.empty()) { + ink_assert(pending_action.empty()); } cache_sm.end_both(); @@ -7269,7 +7269,7 @@ HttpSM::kill_this() plugin_tunnel = nullptr; } - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); ink_release_assert(vc_table.is_table_clear() == true); ink_release_assert(tunnel.is_tunnel_active() == false); @@ -8272,7 +8272,7 @@ HttpSM::get_http_schedule(int event, void * /* data ATS_UNUSED */) if (!plugin_lock) { HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::get_http_schedule); - ink_assert(pending_action.is_empty()); + ink_assert(pending_action.empty()); pending_action = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10)); return 0; } else { diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h index 9dd737541c7..8a8664e79fc 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -44,6 +44,7 @@ #include "../ProxyTransaction.h" #include "HdrUtils.h" #include "tscore/History.h" +#include "tscore/PendingAction.h" #define HTTP_API_CONTINUE (INK_API_EVENT_EVENTS_START + 0) #define HTTP_API_ERROR (INK_API_EVENT_EVENTS_START + 1) @@ -168,54 +169,6 @@ enum HttpPluginTunnel_t { class PluginVCCore; -class PendingAction -{ -public: - bool - is_empty() const - { - return pending_action == nullptr; - } - PendingAction & - operator=(Action *b) - { - // Don't do anything if the new action is _DONE - if (b != ACTION_RESULT_DONE) { - if (b != pending_action && pending_action != nullptr) { - pending_action->cancel(); - } - pending_action = b; - } - return *this; - } - Continuation * - get_continuation() const - { - return pending_action ? pending_action->continuation : nullptr; - } - Action * - get() const - { - return pending_action; - } - void - clear_if_action_is(Action *current_action) - { - if (current_action == pending_action) { - pending_action = nullptr; - } - } - ~PendingAction() - { - if (pending_action) { - pending_action->cancel(); - } - } - -private: - Action *pending_action = nullptr; -}; - class PostDataBuffers { public: