diff --git a/proxy/ProxyClientSession.cc b/proxy/ProxyClientSession.cc index 44d8dcad0c0..1fe9daac019 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 -1; + return 0; } } @@ -160,7 +160,7 @@ ProxyClientSession::state_api_callout(int event, void *data) return 0; } -int +void ProxyClientSession::do_api_callout(TSHttpHookID id) { ink_assert(id == TS_HTTP_SSN_START_HOOK || id == TS_HTTP_SSN_CLOSE_HOOK); @@ -171,11 +171,10 @@ ProxyClientSession::do_api_callout(TSHttpHookID id) if (this->hooks_on && this->has_hooks()) { SET_HANDLER(&ProxyClientSession::state_api_callout); - return this->state_api_callout(EVENT_NONE, nullptr); + 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 5e44262d466..76dc0c60970 100644 --- a/proxy/ProxyClientSession.h +++ b/proxy/ProxyClientSession.h @@ -159,7 +159,7 @@ class ProxyClientSession : public VConnection } // Initiate an API hook invocation. - int do_api_callout(TSHttpHookID id); + void 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 56e5e0e76b1..13b4f7d84f4 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -333,14 +333,13 @@ HttpSM::set_ua_half_close_flag() ua_txn->set_half_close_flag(true); } -inline int +inline void HttpSM::do_api_callout() { if (hooks_set) { - return do_api_callout_internal(); + do_api_callout_internal(); } else { handle_api_return(); - return 0; } } @@ -366,16 +365,7 @@ HttpSM::state_add_to_list(int event, void * /* data ATS_UNUSED */) } t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_START; - 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; - } + do_api_callout(); return EVENT_DONE; } @@ -529,7 +519,6 @@ 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. @@ -1445,7 +1434,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 -1; + return 0; } } else { plugin_lock = false; @@ -5049,12 +5038,12 @@ HttpSM::do_http_server_open(bool raw) return; } -int +void HttpSM::do_api_callout_internal() { if (t_state.backdoor_request) { handle_api_return(); - return 0; + return; } switch (t_state.api_next_action) { @@ -5095,7 +5084,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 0; + return; } else { cur_hook_id = TS_HTTP_TXN_CLOSE_HOOK; } @@ -5107,7 +5096,7 @@ HttpSM::do_api_callout_internal() cur_hook = nullptr; cur_hooks = 0; - return state_api_callout(0, nullptr); + state_api_callout(0, nullptr); } VConnection * @@ -6846,12 +6835,7 @@ HttpSM::kill_this() // the terminate_flag terminate_sm = false; t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_SHUTDOWN; - 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; - } + do_api_callout(); } // 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 5990d96cb81..b9ce2d7fdec 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(); - int do_api_callout(); - int do_api_callout_internal(); + void do_api_callout(); + void 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 f106250ba08..6d7d3de7992 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(); + this->connection_state.release_stream(nullptr); } this->clear_session_active(); diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 27cdd200169..394b299884f 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)}); - stream->initiating_close(); + cstate.delete_stream(stream); } 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,11 +1298,13 @@ Http2ConnectionState::cleanup_streams() if (this->tx_error_code.cls != ProxyErrorClass::NONE) { s->set_tx_error_code(this->tx_error_code); } - s->initiating_close(); + this->delete_stream(s); 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()); @@ -1365,10 +1367,16 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) } void -Http2ConnectionState::release_stream() +Http2ConnectionState::release_stream(Http2Stream *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); @@ -1467,7 +1475,7 @@ Http2ConnectionState::send_data_frames_depends_on_priority() } case Http2SendDataFrameResult::DONE: { dependency_tree->deactivate(node, len); - stream->initiating_close(); + delete_stream(stream); break; } default: @@ -1565,7 +1573,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"); - stream->initiating_close(); + this->delete_stream(stream); return; } @@ -1580,7 +1588,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"); - stream->initiating_close(); + this->delete_stream(stream); } } diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index 104d2f46499..a30990ee2b8 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(); + void release_stream(Http2Stream *stream); void cleanup_streams(); void restart_receiving(Http2Stream *stream); void update_initial_rwnd(Http2WindowSize new_size); @@ -228,12 +228,6 @@ 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 d632a3ac7a6..6cbfe0a0c8e 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -410,6 +410,7 @@ 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(); } } @@ -779,10 +780,8 @@ 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(); + h2_proxy_ssn->connection_state.release_stream(this); // Do not access `_proxy_ssn` in below. It might be freed by `release_stream`. } @@ -932,6 +931,7 @@ void Http2Stream::release(IOBufferReader *r) { super::release(r); + current_reader = nullptr; // State machine is on its own way down. this->do_io_close(); }