diff --git a/proxy/ProxyClientSession.cc b/proxy/ProxyClientSession.cc index 1fe9daac019..44d8dcad0c0 100644 --- a/proxy/ProxyClientSession.cc +++ b/proxy/ProxyClientSession.cc @@ -130,7 +130,7 @@ ProxyClientSession::state_api_callout(int event, void *data) if (!schedule_event) { // Don't bother to schedule is there is already one out. schedule_event = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10)); } - return 0; + return -1; } } @@ -160,7 +160,7 @@ ProxyClientSession::state_api_callout(int event, void *data) return 0; } -void +int ProxyClientSession::do_api_callout(TSHttpHookID id) { ink_assert(id == TS_HTTP_SSN_START_HOOK || id == TS_HTTP_SSN_CLOSE_HOOK); @@ -171,10 +171,11 @@ ProxyClientSession::do_api_callout(TSHttpHookID id) if (this->hooks_on && this->has_hooks()) { SET_HANDLER(&ProxyClientSession::state_api_callout); - this->state_api_callout(EVENT_NONE, nullptr); + return this->state_api_callout(EVENT_NONE, nullptr); } else { this->handle_api_return(TS_EVENT_HTTP_CONTINUE); } + return 0; } void diff --git a/proxy/ProxyClientSession.h b/proxy/ProxyClientSession.h index 76dc0c60970..5e44262d466 100644 --- a/proxy/ProxyClientSession.h +++ b/proxy/ProxyClientSession.h @@ -159,7 +159,7 @@ class ProxyClientSession : public VConnection } // Initiate an API hook invocation. - void do_api_callout(TSHttpHookID id); + int do_api_callout(TSHttpHookID id); // Override if your session protocol allows this. virtual bool diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 13b4f7d84f4..56e5e0e76b1 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -333,13 +333,14 @@ HttpSM::set_ua_half_close_flag() ua_txn->set_half_close_flag(true); } -inline void +inline int HttpSM::do_api_callout() { if (hooks_set) { - do_api_callout_internal(); + return do_api_callout_internal(); } else { handle_api_return(); + return 0; } } @@ -365,7 +366,16 @@ HttpSM::state_add_to_list(int event, void * /* data ATS_UNUSED */) } t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_START; - do_api_callout(); + if (do_api_callout() < 0) { + // Didn't get the hook continuation lock. Clear the read and wait for next event + if (ua_entry->read_vio) { + // Seems like ua_entry->read_vio->disable(); should work, but that was + // not sufficient to stop the state machine from processing IO events until the + // TXN_START hooks had completed + ua_entry->read_vio = ua_entry->vc->do_io_read(nullptr, 0, nullptr); + } + return EVENT_CONT; + } return EVENT_DONE; } @@ -519,6 +529,7 @@ HttpSM::attach_client_session(ProxyClientTransaction *client_vc, IOBufferReader ++reentrancy_count; // Add our state sm to the sm list state_add_to_list(EVENT_NONE, nullptr); + // This is another external entry point and it is possible for the state machine to get terminated // while down the call chain from @c state_add_to_list. So we need to use the reentrancy_count to // prevent cleanup there and do it here as we return to the external caller. @@ -1434,7 +1445,7 @@ plugins required to work with sni_routing. // handler has been changed the value isn't important to the rest of the state machine // but not resetting means there is no way to reliably detect re-entrance to this state with an // outstanding callout. - return 0; + return -1; } } else { plugin_lock = false; @@ -5038,12 +5049,12 @@ HttpSM::do_http_server_open(bool raw) return; } -void +int HttpSM::do_api_callout_internal() { if (t_state.backdoor_request) { handle_api_return(); - return; + return 0; } switch (t_state.api_next_action) { @@ -5084,7 +5095,7 @@ HttpSM::do_api_callout_internal() case HttpTransact::SM_ACTION_API_SM_SHUTDOWN: if (callout_state == HTTP_API_IN_CALLOUT || callout_state == HTTP_API_DEFERED_SERVER_ERROR) { callout_state = HTTP_API_DEFERED_CLOSE; - return; + return 0; } else { cur_hook_id = TS_HTTP_TXN_CLOSE_HOOK; } @@ -5096,7 +5107,7 @@ HttpSM::do_api_callout_internal() cur_hook = nullptr; cur_hooks = 0; - state_api_callout(0, nullptr); + return state_api_callout(0, nullptr); } VConnection * @@ -6835,7 +6846,12 @@ HttpSM::kill_this() // the terminate_flag terminate_sm = false; t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_SHUTDOWN; - do_api_callout(); + if (do_api_callout() < 0) { // Failed to get a continuation lock + // Need to hang out until we can complete the TXN_CLOSE hook + terminate_sm = false; + reentrancy_count--; + return; + } } // The reentrancy_count is still valid up to this point since // the api shutdown hook is asynchronous and double frees can diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h index b9ce2d7fdec..5990d96cb81 100644 --- a/proxy/http/HttpSM.h +++ b/proxy/http/HttpSM.h @@ -466,8 +466,8 @@ class HttpSM : public Continuation void do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_info, bool retry, bool allow_multiple = false); void do_cache_delete_all_alts(Continuation *cont); void do_auth_callout(); - void do_api_callout(); - void do_api_callout_internal(); + int do_api_callout(); + int do_api_callout_internal(); void do_redirect(); void redirect_request(const char *redirect_url, const int redirect_len); void do_drain_request_body(); diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 6d7d3de7992..f106250ba08 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -308,7 +308,7 @@ Http2ClientSession::do_io_close(int alerrno) { SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); - this->connection_state.release_stream(nullptr); + this->connection_state.release_stream(); } this->clear_session_active(); diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 394b299884f..27cdd200169 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/proxy/http2/Http2ConnectionState.cc @@ -531,7 +531,7 @@ rcv_rst_stream_frame(Http2ConnectionState &cstate, const Http2Frame &frame) Http2StreamDebug(cstate.ua_session, stream_id, "RST_STREAM: Error Code: %u", rst_stream.error_code); stream->set_rx_error_code({ProxyErrorClass::TXN, static_cast(rst_stream.error_code)}); - cstate.delete_stream(stream); + stream->initiating_close(); } return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); @@ -1000,8 +1000,8 @@ Http2ConnectionState::main_event_handler(int event, void *edata) ink_assert(this->fini_received == false); this->fini_received = true; cleanup_streams(); + release_stream(); SET_HANDLER(&Http2ConnectionState::state_closed); - this->release_stream(nullptr); } break; case HTTP2_SESSION_EVENT_XMIT: { @@ -1298,13 +1298,11 @@ Http2ConnectionState::cleanup_streams() if (this->tx_error_code.cls != ProxyErrorClass::NONE) { s->set_tx_error_code(this->tx_error_code); } - this->delete_stream(s); + s->initiating_close(); ink_assert(s != next); s = next; } - ink_assert(stream_list.empty()); - if (!is_state_closed()) { SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread()); @@ -1367,16 +1365,10 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) } void -Http2ConnectionState::release_stream(Http2Stream *stream) +Http2ConnectionState::release_stream() { REMEMBER(NO_EVENT, this->recursion) - if (stream) { - // Decrement total_client_streams_count here, because it's a counter include streams in the process of shutting down. - // Other counters (client_streams_in_count/client_streams_out_count) are already decremented in delete_stream(). - --total_client_streams_count; - } - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); if (this->ua_session) { ink_assert(this->mutex == ua_session->mutex); @@ -1475,7 +1467,7 @@ Http2ConnectionState::send_data_frames_depends_on_priority() } case Http2SendDataFrameResult::DONE: { dependency_tree->deactivate(node, len); - delete_stream(stream); + stream->initiating_close(); break; } default: @@ -1573,7 +1565,7 @@ Http2ConnectionState::send_data_frames(Http2Stream *stream) if (stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL || stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_CLOSED) { Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown half closed local stream"); - this->delete_stream(stream); + stream->initiating_close(); return; } @@ -1588,7 +1580,7 @@ Http2ConnectionState::send_data_frames(Http2Stream *stream) // RST_STREAM and WINDOW_UPDATE. // See 'closed' state written at [RFC 7540] 5.1. Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown stream"); - this->delete_stream(stream); + stream->initiating_close(); } } diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index a30990ee2b8..104d2f46499 100644 --- a/proxy/http2/Http2ConnectionState.h +++ b/proxy/http2/Http2ConnectionState.h @@ -176,7 +176,7 @@ class Http2ConnectionState : public Continuation Http2Stream *find_stream(Http2StreamId id) const; void restart_streams(); bool delete_stream(Http2Stream *stream); - void release_stream(Http2Stream *stream); + void release_stream(); void cleanup_streams(); void restart_receiving(Http2Stream *stream); void update_initial_rwnd(Http2WindowSize new_size); @@ -228,6 +228,12 @@ class Http2ConnectionState : public Continuation return client_streams_in_count; } + void + decrement_stream_count() + { + --total_client_streams_count; + } + double get_stream_error_rate() const { diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 6cbfe0a0c8e..d632a3ac7a6 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -410,7 +410,6 @@ Http2Stream::terminate_if_possible() REMEMBER(NO_EVENT, this->reentrancy_count); Http2ClientSession *h2_proxy_ssn = static_cast(parent); SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex, this_ethread()); - h2_proxy_ssn->connection_state.delete_stream(this); destroy(); } } @@ -780,8 +779,10 @@ Http2Stream::destroy() // In many cases, this has been called earlier, so this call is a no-op h2_proxy_ssn->connection_state.delete_stream(this); + h2_proxy_ssn->connection_state.decrement_stream_count(); + // Update session's stream counts, so it accurately goes into keep-alive state - h2_proxy_ssn->connection_state.release_stream(this); + h2_proxy_ssn->connection_state.release_stream(); // Do not access `_proxy_ssn` in below. It might be freed by `release_stream`. } @@ -931,7 +932,6 @@ void Http2Stream::release(IOBufferReader *r) { super::release(r); - current_reader = nullptr; // State machine is on its own way down. this->do_io_close(); }