From 7cce49ec87bfb0327c96ba58a29c845a716e9b9a Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 31 Mar 2021 20:48:07 +0000 Subject: [PATCH 1/2] Add class to normalize handling of pending action --- proxy/http/HttpSM.cc | 139 +++++++++++-------------------------------- proxy/http/HttpSM.h | 50 +++++++++++++++- 2 files changed, 82 insertions(+), 107 deletions(-) diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 36ae609cb63..feafe0af5ff 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -1474,9 +1474,6 @@ HttpSM::state_api_callout(int event, void *data) // This is a reschedule via the tunnel. Just fall through // case EVENT_INTERVAL: - if (data != pending_action) { - pending_action->cancel(); - } pending_action = nullptr; // FALLTHROUGH case EVENT_NONE: @@ -1527,7 +1524,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_assert(pending_action == nullptr); + ink_release_assert(pending_action == nullptr); pending_action = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10)); return -1; } @@ -2343,12 +2340,8 @@ HttpSM::state_hostdb_lookup(int event, void *data) opt.timeout = (t_state.api_txn_dns_timeout_value != -1) ? t_state.api_txn_dns_timeout_value : 0; 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); - Action *dns_lookup_action_handle = - hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); - if (dns_lookup_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = dns_lookup_action_handle; - } else { + pending_action = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); + if (pending_action == nullptr) { call_transact_and_set_next_state(nullptr); } } break; @@ -2482,13 +2475,13 @@ HttpSM::state_cache_open_write(int event, void *data) // Make sure we are on the "right" thread if (ua_txn) { - if (pending_action) { - pending_action->cancel(); - } - if ((pending_action = ua_txn->adjust_thread(this, event, data))) { + pending_action = ua_txn->adjust_thread(this, event, data); + if (pending_action != nullptr) { HTTP_INCREMENT_DYN_STAT(http_cache_open_write_adjust_thread_stat); return 0; // Go away if we reschedule } + NetVConnection *vc = ua_txn->get_netvc(); + ink_release_assert(vc && vc->thread == this_ethread()); } milestones[TS_MILESTONE_CACHE_OPEN_WRITE_END] = Thread::get_hrtime(); @@ -4149,13 +4142,7 @@ HttpSM::do_remap_request(bool run_inline) } SMDebug("url_rewrite", "Found a remap map entry for [%" PRId64 "], attempting to remap request and call any plugins", sm_id); - Action *remap_action_handle = remapProcessor.perform_remap(this, &t_state); - - if (remap_action_handle != ACTION_RESULT_DONE) { - SMDebug("url_rewrite", "Still more remapping needed for [%" PRId64 "]", sm_id); - ink_assert(!pending_action); - pending_action = remap_action_handle; - } + pending_action = remapProcessor.perform_remap(this, &t_state); return; } @@ -4181,13 +4168,8 @@ HttpSM::do_hostdb_lookup() if (t_state.api_txn_dns_timeout_value != -1) { opt.timeout = t_state.api_txn_dns_timeout_value; } - Action *srv_lookup_action_handle = - hostDBProcessor.getSRVbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_srv_info, d, 0, opt); - - if (srv_lookup_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = srv_lookup_action_handle; - } else { + pending_action = hostDBProcessor.getSRVbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_srv_info, d, 0, opt); + if (pending_action == nullptr) { 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 : @@ -4199,12 +4181,8 @@ HttpSM::do_hostdb_lookup() 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); - Action *dns_lookup_action_handle = - hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); - if (dns_lookup_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = dns_lookup_action_handle; - } else { + pending_action = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, host_name, 0, opt); + if (pending_action == nullptr) { call_transact_and_set_next_state(nullptr); } } @@ -4235,13 +4213,9 @@ HttpSM::do_hostdb_lookup() 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); - Action *dns_lookup_action_handle = hostDBProcessor.getbyname_imm(this, (cb_process_result_pfn)&HttpSM::process_hostdb_info, - t_state.dns_info.lookup_name, 0, opt); - - if (dns_lookup_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = dns_lookup_action_handle; - } else { + 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 == nullptr) { call_transact_and_set_next_state(nullptr); } return; @@ -4260,12 +4234,8 @@ HttpSM::do_hostdb_reverse_lookup() IpEndpoint addr; ats_ip_pton(t_state.dns_info.lookup_name, &addr.sa); - Action *dns_lookup_action_handle = hostDBProcessor.getbyaddr_re(this, &addr.sa); + pending_action = hostDBProcessor.getbyaddr_re(this, &addr.sa); - if (dns_lookup_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = dns_lookup_action_handle; - } return; } @@ -4674,7 +4644,7 @@ HttpSM::do_cache_lookup_and_read() HttpCacheKey key; Cache::generate_key(&key, c_url, t_state.txn_conf->cache_generation_number); - Action *cache_action_handle = cache_sm.open_read( + pending_action = cache_sm.open_read( &key, c_url, &t_state.hdr_info.client_request, t_state.txn_conf, static_cast((t_state.cache_control.pin_in_cache_for < 0) ? 0 : t_state.cache_control.pin_in_cache_for)); // @@ -4683,11 +4653,7 @@ HttpSM::do_cache_lookup_and_read() // optimize the typical open_read/open_read failed/open_write // sequence. // - if (cache_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = cache_action_handle; - } - REMEMBER((long)pending_action, reentrancy_count); + REMEMBER((long)pending_action.get(), reentrancy_count); return; } @@ -4701,17 +4667,9 @@ HttpSM::do_cache_delete_all_alts(Continuation *cont) SMDebug("http_seq", "[HttpSM::do_cache_delete_all_alts] Issuing cache delete for %s", t_state.cache_info.lookup_url->string_get_ref()); - Action *cache_action_handle = nullptr; - HttpCacheKey key; Cache::generate_key(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_generation_number); - cache_action_handle = cacheProcessor.remove(cont, &key); - if (cont != nullptr) { - if (cache_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = cache_action_handle; - } - } + pending_action = cacheProcessor.remove(cont, &key); return; } @@ -4761,7 +4719,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); + ink_assert(pending_action == nullptr); if (t_state.redirect_info.redirect_in_process) { o_url = &(t_state.redirect_info.original_url); @@ -4790,15 +4748,10 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in HttpCacheKey key; Cache::generate_key(&key, s_url, t_state.txn_conf->cache_generation_number); - Action *cache_action_handle = + pending_action = c_sm->open_write(&key, s_url, &t_state.hdr_info.client_request, object_read_info, static_cast((t_state.cache_control.pin_in_cache_for < 0) ? 0 : t_state.cache_control.pin_in_cache_for), retry, allow_multiple); - - if (cache_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = cache_action_handle; - } } void @@ -4898,15 +4851,9 @@ HttpSM::do_http_server_open(bool raw) auto fam_name = ats_ip_family_name(ip_family); SMDebug("http_track", "entered inside do_http_server_open ][%.*s]", static_cast(fam_name.size()), fam_name.data()); - // Make sure we are on the "right" thread - if (ua_txn) { - if ((pending_action = ua_txn->adjust_thread(this, EVENT_INTERVAL, nullptr))) { - HTTP_INCREMENT_DYN_STAT(http_origin_connect_adjust_thread_stat); - return; // Go away if we reschedule - } - } + NetVConnection *vc = ua_txn->get_netvc(); + ink_release_assert(vc && vc->thread == this_ethread()); pending_action = nullptr; - ink_assert(server_entry == nullptr); // Clean up connection tracking info if any. Need to do it now so the selected group // is consistent with the actual upstream in case of retry. @@ -5152,8 +5099,6 @@ HttpSM::do_http_server_open(bool raw) } // We did not manage to get an existing session and need to open a new connection - Action *connect_action_handle; - NetVCOptions opt; opt.f_blocking_connect = false; opt.set_sock_param(t_state.txn_conf->sock_recv_buffer_size_out, t_state.txn_conf->sock_send_buffer_size_out, @@ -5246,19 +5191,14 @@ HttpSM::do_http_server_open(bool raw) opt.set_ssl_servername(t_state.server_info.name); } - connect_action_handle = sslNetProcessor.connect_re(this, // state machine - &t_state.current.server->dst_addr.sa, // addr + port - &opt); + pending_action = sslNetProcessor.connect_re(this, // state machine + &t_state.current.server->dst_addr.sa, // addr + port + &opt); } else { SMDebug("http", "calling netProcessor.connect_re"); - connect_action_handle = netProcessor.connect_re(this, // state machine - &t_state.current.server->dst_addr.sa, // addr + port - &opt); - } - - if (connect_action_handle != ACTION_RESULT_DONE) { - ink_assert(!pending_action); - pending_action = connect_action_handle; + pending_action = netProcessor.connect_re(this, // state machine + &t_state.current.server->dst_addr.sa, // addr + port + &opt); } return; @@ -7002,10 +6942,9 @@ 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) { - pending_action->cancel(); + if (callout_state == HTTP_API_NO_CALLOUT && pending_action != nullptr) { pending_action = nullptr; - } else if (pending_action) { + } else if (pending_action != nullptr) { ink_assert(pending_action == nullptr); } @@ -7068,10 +7007,7 @@ HttpSM::kill_this() // then the value of kill_this_async_done has changed so // we must check it again if (kill_this_async_done == true) { - if (pending_action) { - pending_action->cancel(); - pending_action = nullptr; - } + pending_action = nullptr; if (t_state.http_config_param->enable_http_stats) { update_stats(); } @@ -7702,17 +7638,10 @@ HttpSM::set_next_state() break; } - case HttpTransact::SM_ACTION_INTERNAL_REQUEST: { + case HttpTransact::SM_ACTION_INTERNAL_REQUEST: HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_handle_stat_page); - Action *action_handle = statPagesManager.handle_http(this, &t_state.hdr_info.client_request); - - if (action_handle != ACTION_RESULT_DONE) { - ink_assert(pending_action == nullptr); - pending_action = action_handle; - } - + pending_action = statPagesManager.handle_http(this, &t_state.hdr_info.client_request); break; - } case HttpTransact::SM_ACTION_ORIGIN_SERVER_RR_MARK_DOWN: { HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_mark_os_down); diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h index 8a82ebb8980..e5a75de94a0 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -166,6 +166,52 @@ enum HttpPluginTunnel_t { class PluginVCCore; +class PendingAction +{ +public: + bool + operator==(Action *b) + { + return b == pending_action; + } + bool + operator!=(Action *b) + { + return b != pending_action; + } + 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; + } + Action * + operator->() + { + return pending_action; + } + Action * + get() + { + return pending_action; + } + ~PendingAction() + { + if (pending_action) { + pending_action->cancel(); + } + } + +private: + Action *pending_action = nullptr; +}; + class PostDataBuffers { public: @@ -387,8 +433,8 @@ class HttpSM : public Continuation, public PluginUserArgs HttpCacheSM transform_cache_sm; HttpSMHandler default_handler = nullptr; - Action *pending_action = nullptr; - Continuation *schedule_cont = nullptr; + PendingAction pending_action; + Continuation *schedule_cont = nullptr; HTTPParser http_parser; void start_sub_sm(); From ab4102fc0970f9c82b01f50651f0456da15b010c Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Tue, 6 Apr 2021 14:43:55 +0000 Subject: [PATCH 2/2] Make the PendingAction class less pointer-ish --- proxy/http/HttpSM.cc | 38 +++++++++++++++++++------------------- proxy/http/HttpSM.h | 17 ++++++----------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index feafe0af5ff..12fd4e32a05 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -1524,7 +1524,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 == nullptr); + ink_release_assert(pending_action.is_empty()); pending_action = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10)); return -1; } @@ -1804,7 +1804,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 == nullptr); + pending_action.is_empty()); if (event != NET_EVENT_OPEN) { pending_action = nullptr; } @@ -1825,7 +1825,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 == nullptr || pending_action->continuation == vc->get_action()->continuation); + ink_release_assert(pending_action.is_empty() || pending_action.get_continuation() == vc->get_action()->continuation); pending_action = nullptr; session->new_connection(vc, nullptr, nullptr); @@ -2341,7 +2341,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 == nullptr) { + if (pending_action.is_empty()) { call_transact_and_set_next_state(nullptr); } } break; @@ -2476,7 +2476,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 != nullptr) { + if (!pending_action.is_empty()) { HTTP_INCREMENT_DYN_STAT(http_cache_open_write_adjust_thread_stat); return 0; // Go away if we reschedule } @@ -4151,7 +4151,7 @@ void HttpSM::do_hostdb_lookup() { ink_assert(t_state.dns_info.lookup_name != nullptr); - ink_assert(pending_action == nullptr); + ink_assert(pending_action.is_empty()); milestones[TS_MILESTONE_DNS_LOOKUP_BEGIN] = Thread::get_hrtime(); @@ -4169,7 +4169,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 == nullptr) { + if (pending_action.is_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 : @@ -4182,7 +4182,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 == nullptr) { + if (pending_action.is_empty()) { call_transact_and_set_next_state(nullptr); } } @@ -4215,7 +4215,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 == nullptr) { + if (pending_action.is_empty()) { call_transact_and_set_next_state(nullptr); } return; @@ -4228,7 +4228,7 @@ void HttpSM::do_hostdb_reverse_lookup() { ink_assert(t_state.dns_info.lookup_name != nullptr); - ink_assert(pending_action == nullptr); + ink_assert(pending_action.is_empty()); SMDebug("http_seq", "[HttpSM::do_hostdb_reverse_lookup] Doing reverse DNS Lookup"); @@ -4621,7 +4621,7 @@ HttpSM::do_cache_lookup_and_read() { // TODO decide whether to uncomment after finish testing redirect // ink_assert(server_session == NULL); - ink_assert(pending_action == nullptr); + ink_assert(pending_action.is_empty()); HTTP_INCREMENT_DYN_STAT(http_cache_lookups_stat); @@ -4719,7 +4719,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 == nullptr); + ink_assert(pending_action.is_empty()); if (t_state.redirect_info.redirect_in_process) { o_url = &(t_state.redirect_info.original_url); @@ -4865,7 +4865,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 == nullptr); + ink_assert(pending_action.is_empty()); ink_assert(t_state.current.server->dst_addr.port() != 0); char addrbuf[INET6_ADDRPORTSTRLEN]; @@ -5082,7 +5082,7 @@ HttpSM::do_http_server_open(bool raw) if (ccount > t_state.txn_conf->outbound_conntrack.max) { ct_state.release(); - ink_assert(pending_action == nullptr); // in case of reschedule must not have already pending. + ink_assert(pending_action.is_empty()); // in case of reschedule must not have already pending. ct_state.blocked(); HTTP_INCREMENT_DYN_STAT(http_origin_connections_throttled_stat); @@ -6942,10 +6942,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 != nullptr) { + if (callout_state == HTTP_API_NO_CALLOUT && !pending_action.is_empty()) { pending_action = nullptr; - } else if (pending_action != nullptr) { - ink_assert(pending_action == nullptr); + } else if (!pending_action.is_empty()) { + ink_assert(pending_action.is_empty()); } cache_sm.end_both(); @@ -7041,7 +7041,7 @@ HttpSM::kill_this() plugin_tunnel = nullptr; } - ink_assert(pending_action == nullptr); + ink_assert(pending_action.is_empty()); ink_release_assert(vc_table.is_table_clear() == true); ink_release_assert(tunnel.is_tunnel_active() == false); @@ -8041,7 +8041,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 == nullptr); + ink_assert(pending_action.is_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 e5a75de94a0..34a5b0f85ee 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -170,14 +170,9 @@ class PendingAction { public: bool - operator==(Action *b) + is_empty() const { - return b == pending_action; - } - bool - operator!=(Action *b) - { - return b != pending_action; + return pending_action == nullptr; } PendingAction & operator=(Action *b) @@ -191,13 +186,13 @@ class PendingAction } return *this; } - Action * - operator->() + Continuation * + get_continuation() const { - return pending_action; + return pending_action ? pending_action->continuation : nullptr; } Action * - get() + get() const { return pending_action; }