From 0b86b5344a1ead345bce74f5e7d1bf8133acd0fa Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Fri, 14 May 2021 16:09:25 +0900 Subject: [PATCH 1/4] Introduce HTTP/2 Session States --- doc/admin-guide/files/records.config.en.rst | 9 - proxy/http2/HTTP2.cc | 2 - proxy/http2/HTTP2.h | 1 - proxy/http2/Http2ClientSession.cc | 632 ++++++++++++++++---- proxy/http2/Http2ClientSession.h | 125 ++-- proxy/http2/Http2ConnectionState.cc | 349 +++-------- proxy/http2/Http2ConnectionState.h | 90 +-- proxy/http2/Http2Stream.cc | 78 ++- src/traffic_server/InkAPI.cc | 2 +- 9 files changed, 729 insertions(+), 559 deletions(-) diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index 3eccb04a4d6..681b952971f 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/doc/admin-guide/files/records.config.en.rst @@ -4010,15 +4010,6 @@ HTTP/2 Configuration misconfigured or misbehaving clients are opening a large number of connections without submitting requests. -.. ts:cv:: CONFIG proxy.config.http2.zombie_debug_timeout_in INT 0 - :reloadable: - - This timeout enables the zombie debugging feature. If it is non-zero, it sets a zombie event to go off that - many seconds in the future when the HTTP2 session reaches one but not both of the terminating events, i.e received - a close event (via client goaway or timeout) and the number of active streams has gone to zero. If the event is executed, - the |TS| process will assert. This mechanism is useful to debug potential leaks in the HTTP2 Stream and Session - processing. - .. ts:cv:: CONFIG proxy.config.http2.push_diary_size INT 256 :reloadable: diff --git a/proxy/http2/HTTP2.cc b/proxy/http2/HTTP2.cc index 648609cbb80..cf35767dd36 100644 --- a/proxy/http2/HTTP2.cc +++ b/proxy/http2/HTTP2.cc @@ -793,7 +793,6 @@ uint32_t Http2::accept_no_activity_timeout = 120; uint32_t Http2::no_activity_timeout_in = 120; uint32_t Http2::active_timeout_in = 0; uint32_t Http2::push_diary_size = 256; -uint32_t Http2::zombie_timeout_in = 0; float Http2::stream_error_rate_threshold = 0.1; uint32_t Http2::max_settings_per_frame = 7; uint32_t Http2::max_settings_per_minute = 14; @@ -823,7 +822,6 @@ Http2::init() REC_EstablishStaticConfigInt32U(no_activity_timeout_in, "proxy.config.http2.no_activity_timeout_in"); REC_EstablishStaticConfigInt32U(active_timeout_in, "proxy.config.http2.active_timeout_in"); REC_EstablishStaticConfigInt32U(push_diary_size, "proxy.config.http2.push_diary_size"); - REC_EstablishStaticConfigInt32U(zombie_timeout_in, "proxy.config.http2.zombie_debug_timeout_in"); REC_EstablishStaticConfigFloat(stream_error_rate_threshold, "proxy.config.http2.stream_error_rate_threshold"); REC_EstablishStaticConfigInt32U(max_settings_per_frame, "proxy.config.http2.max_settings_per_frame"); REC_EstablishStaticConfigInt32U(max_settings_per_minute, "proxy.config.http2.max_settings_per_minute"); diff --git a/proxy/http2/HTTP2.h b/proxy/http2/HTTP2.h index d2eed22ffe3..6ae2e2fbab3 100644 --- a/proxy/http2/HTTP2.h +++ b/proxy/http2/HTTP2.h @@ -392,7 +392,6 @@ class Http2 static uint32_t no_activity_timeout_in; static uint32_t active_timeout_in; static uint32_t push_diary_size; - static uint32_t zombie_timeout_in; static float stream_error_rate_threshold; static uint32_t max_settings_per_frame; static uint32_t max_settings_per_minute; diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 1834e1c8b4e..46e98e581e2 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -32,11 +32,10 @@ this->remember(MakeSourceLocation(), e, r); \ } -#define STATE_ENTER(state_name, event) \ - do { \ - REMEMBER(event, this->recursion) \ - SsnDebug(this, "http2_cs", "[%" PRId64 "] [%s, %s]", this->connection_id(), #state_name, \ - HttpDebugNames::get_event_name(event)); \ +#define STATE_ENTER(state_name, event) \ + do { \ + REMEMBER(event, this->recursion) \ + SsnDebug(this, "http2_cs", "[%" PRId64 "] [%s, %s]", this->connection_id(), #state_name, get_vc_event_name(event)); \ } while (0) #define Http2SsnDebug(fmt, ...) SsnDebug(this, "http2_cs", "[%" PRId64 "] " fmt, this->connection_id(), ##__VA_ARGS__) @@ -47,6 +46,12 @@ this->session_handler = (handler); \ } while (0) +#define HTTP2_SET_SESSION_STATE(state) \ + do { \ + REMEMBER(NO_EVENT, this->recursion); \ + session_state = (state); \ + } while (0) + ClassAllocator http2ClientSessionAllocator("http2ClientSessionAllocator"); // memcpy the requested bytes from the IOBufferReader, returning how many were @@ -60,18 +65,14 @@ copy_from_buffer_reader(void *dst, IOBufferReader *reader, unsigned nbytes) return end - static_cast(dst); } -static int -send_connection_event(Continuation *cont, int event, void *edata) -{ - SCOPED_MUTEX_LOCK(lock, cont->mutex, this_ethread()); - return cont->handleEvent(event, edata); -} - Http2ClientSession::Http2ClientSession() : super() {} void Http2ClientSession::destroy() { + ink_assert(session_state == &Http2ClientSession::state_closed); + ink_assert(_vc == nullptr); + if (!in_destroy) { in_destroy = true; REMEMBER(NO_EVENT, this->recursion) @@ -81,21 +82,15 @@ Http2ClientSession::destroy() } } +/** + Should be called from ProxySession::handle_api_return via destroy() + */ void Http2ClientSession::free() { - if (_vc) { - _vc->do_io_close(); - _vc = nullptr; - } - - // Make sure the we are at the bottom of the stack - if (connection_state.is_recursing() || this->recursion != 0) { - // Note that we are ready to be cleaned up - // One of the event handlers will catch it - kill_me = true; - return; - } + // _vc should be closed before moving to state_closed + ink_assert(_vc == nullptr); + ink_assert(recursion == 0); REMEMBER(NO_EVENT, this->recursion) Http2SsnDebug("session free"); @@ -105,6 +100,11 @@ Http2ClientSession::free() this->_reenable_event = nullptr; } + if (_graceful_shutdown_event) { + _graceful_shutdown_event->cancel(); + _graceful_shutdown_event = nullptr; + } + // Don't free active ProxySession ink_release_assert(is_active() == false); @@ -156,8 +156,6 @@ Http2ClientSession::free() } } - ink_release_assert(this->_vc == nullptr); - delete _h2_pushed_urls; this->connection_state.destroy(); @@ -172,10 +170,14 @@ Http2ClientSession::start() SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); SET_HANDLER(&Http2ClientSession::main_event_handler); + HTTP2_SET_SESSION_STATE(&Http2ClientSession::state_open); HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_read_connection_preface); + Http2SsnDebug("state_open"); - VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer); - write_vio = this->do_io_write(this, INT64_MAX, this->_write_buffer_reader); + ink_assert(_vc != nullptr); + + read_vio = _vc->do_io_read(this, INT64_MAX, this->read_buffer); + write_vio = _vc->do_io_write(this, INT64_MAX, this->_write_buffer_reader); this->connection_state.init(this); this->connection_state.send_connection_preface(); @@ -201,8 +203,6 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB this->mutex = new_vc->mutex; this->in_destroy = false; - this->connection_state.mutex = this->mutex; - SSLNetVConnection *ssl_vc = dynamic_cast(new_vc); if (ssl_vc != nullptr) { this->read_from_early_data = ssl_vc->read_from_early_data; @@ -228,39 +228,58 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB do_api_callout(TS_HTTP_SSN_START_HOOK); } -// XXX Currently, we don't have a half-closed state, but we will need to -// implement that. After we send a GOAWAY, there -// are scenarios where we would like to complete the outstanding streams. - -void -Http2ClientSession::do_io_close(int alerrno) +/** + Http2Session's VConnection APIs should not be called. + Because the relation of Http2Session and Http2Transaction is 1 to many. + */ +VIO * +Http2ClientSession::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) { - REMEMBER(NO_EVENT, this->recursion) - Http2SsnDebug("session closed"); + ink_abort("Do NOT Call"); + return nullptr; +} - ink_assert(this->mutex->thread_holding == this_ethread()); - send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_FINI, this); +VIO * +Http2ClientSession::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *reader, bool owner) +{ + ink_abort("Do NOT Call"); + return nullptr; +} - this->connection_state.release_stream(); +/** + Actually, this should not be called. Because nobody call do_io_read() or do_io_write(). + But this is called from ProxySession::handle_api_return() by just historical reason. - this->clear_session_active(); + TODO: Get rid of do_io_close call from ProxySession + */ +void +Http2ClientSession::do_io_close(int lerrno) +{ + REMEMBER(NO_EVENT, this->recursion); + handleEvent(VC_EVENT_ERROR); +} - // Clean up the write VIO in case of inactivity timeout - this->do_io_write(this, 0, nullptr); +void +Http2ClientSession::do_io_shutdown(ShutdownHowTo_t howto) +{ + ink_abort("Do NOT Call"); } void -Http2ClientSession::set_half_close_local_flag(bool flag) +Http2ClientSession::reenable(VIO *vio) { - if (!half_close_local && flag) { - Http2SsnDebug("session half-close local"); - } - half_close_local = flag; + ink_abort("Do NOT Call"); } int64_t Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush) { + if (!is_state_writable()) { + Http2SsnDebug("unexpected state to xmit frame"); + ink_assert(false); + return -1; + } + int64_t len = frame.write_to(this->write_buffer); this->_pending_sending_data_size += len; // Force flush for some cases @@ -289,97 +308,209 @@ Http2ClientSession::flush() } } +/** + Continuation Handler + */ int Http2ClientSession::main_event_handler(int event, void *edata) { ink_assert(this->mutex->thread_holding == this_ethread()); - int retval; - - recursion++; Event *e = static_cast(edata); if (e == schedule_event) { schedule_event = nullptr; } + recursion++; + int retval = (this->*session_state)(event, edata); + recursion--; + + if (recursion == 0 && session_state == &Http2ClientSession::state_closed) { + destroy(); + } + + return retval; +} + +/** + Session State - Default state of handling VC events + */ +int +Http2ClientSession::state_open(int event, void *edata) +{ + STATE_ENTER(state_open, event); + + int res = EVENT_DONE; + switch (event) { + case HTTP2_SESSION_EVENT_REENABLE: { + res = _handle_ssn_reenable_event(event, edata); + break; + } + case VC_EVENT_READ_READY: case VC_EVENT_READ_COMPLETE: - case VC_EVENT_READ_READY: { - bool is_zombie = connection_state.get_zombie_event() != nullptr; - retval = (this->*session_handler)(event, edata); - if (is_zombie && connection_state.get_zombie_event() != nullptr) { - Warning("Processed read event for zombie session %" PRId64, connection_id()); - } + case VC_EVENT_WRITE_READY: + case VC_EVENT_WRITE_COMPLETE: + case VC_EVENT_ACTIVE_TIMEOUT: + case VC_EVENT_INACTIVITY_TIMEOUT: + case VC_EVENT_ERROR: + case VC_EVENT_EOS: { + res = _handle_vc_event(event, edata); break; } - - case HTTP2_SESSION_EVENT_REENABLE: - // VIO will be reenableed in this handler - retval = (this->*session_handler)(VC_EVENT_READ_READY, static_cast(e->cookie)); - // Clear the event after calling session_handler to not reschedule REENABLE in it - this->_reenable_event = nullptr; + default: + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); break; + } + + if (_should_start_graceful_shutdown()) { + HTTP2_SET_SESSION_STATE(&Http2ClientSession::state_graceful_shutdown); + connection_state.send_goaway_frame(INT32_MAX, Http2ErrorCode::HTTP2_ERROR_NO_ERROR); + + // After allowing time for any in-flight stream creation (at least one round-trip time), + _graceful_shutdown_event = this_ethread()->schedule_in_local(this, HRTIME_SECONDS(2), HTTP2_SESSION_EVENT_GRACEFUL_SHUTDOWN); + } + + // Q: should we call _vc->add_to_keep_alive() if there is not stream? + + return res; +} + +/** + Session State - Waiting all transaction is done. When all tranraction is done, move to state_closed. + Read and Write operation will be processed like state_open, but we can't create any new stream in this state. + */ +int +Http2ClientSession::state_graceful_shutdown(int event, void *edata) +{ + STATE_ENTER(state_graceful_shutdown, event); + + int res = EVENT_DONE; + + switch (event) { + case HTTP2_SESSION_EVENT_GRACEFUL_SHUTDOWN: { + REMEMBER(NO_EVENT, recursion); + _graceful_shutdown_event = nullptr; + Http2ErrorCode err = connection_state.shutdown_reason; + if (err == Http2ErrorCode::HTTP2_ERROR_MAX) { + err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR; + } + connection_state.send_goaway_frame(connection_state.get_latest_stream_id_in(), err); + break; + } + case HTTP2_SESSION_EVENT_REENABLE: { + res = _handle_ssn_reenable_event(event, edata); + break; + } + case VC_EVENT_READ_READY: + case VC_EVENT_READ_COMPLETE: + case VC_EVENT_WRITE_READY: + case VC_EVENT_WRITE_COMPLETE: case VC_EVENT_ACTIVE_TIMEOUT: case VC_EVENT_INACTIVITY_TIMEOUT: case VC_EVENT_ERROR: - case VC_EVENT_EOS: - Http2SsnDebug("Closing event %d", event); - this->set_dying_event(event); - this->do_io_close(); - retval = 0; + case VC_EVENT_EOS: { + res = _handle_vc_event(event, edata); break; + } + default: + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); + } - case VC_EVENT_WRITE_READY: - case VC_EVENT_WRITE_COMPLETE: - this->connection_state.restart_streams(); - if ((Thread::get_hrtime() >= this->_write_buffer_last_flush + HRTIME_MSECONDS(this->_write_time_threshold))) { - this->flush(); - } - retval = 0; + if (connection_state.get_client_stream_count() == 0 && !_write_buffer_reader->is_read_avail_more_than(0)) { + REMEMBER(event, recursion); + Http2SsnDebug("state_closed - all stream is gone"); + _set_state_closed(); + } + + return res; +} + +/** + Session State - Waiting all data (especially GOAWAY frame) in the _write_buffer is sent + */ +int +Http2ClientSession::state_goaway(int event, void *edata) +{ + STATE_ENTER(state_goaway, event); + + switch (event) { + case VC_EVENT_WRITE_READY: { + // do nothing break; + } + case VC_EVENT_WRITE_COMPLETE: + case VC_EVENT_ACTIVE_TIMEOUT: + case VC_EVENT_INACTIVITY_TIMEOUT: + case VC_EVENT_ERROR: + case VC_EVENT_EOS: { + REMEMBER(event, recursion); + Http2SsnDebug("state_closed by %s (%d)", get_vc_event_name(event), event); + _set_state_closed(); - case HTTP2_SESSION_EVENT_XMIT: + break; + } default: - Http2SsnDebug("unexpected event=%d edata=%p", event, edata); - ink_release_assert(0); - retval = 0; + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); break; } - if (!this->is_draining() && this->connection_state.get_shutdown_reason() == Http2ErrorCode::HTTP2_ERROR_MAX) { - this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NONE); - } + return EVENT_DONE; +} - if (this->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NONE) { - if (this->is_draining()) { // For a case we already checked Connection header and it didn't exist - Http2SsnDebug("Preparing for graceful shutdown because of draining state"); - this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED); - } else if (this->connection_state.get_stream_error_rate() > - Http2::stream_error_rate_threshold) { // For a case many stream errors happened - ip_port_text_buffer ipb; - const char *client_ip = ats_ip_ntop(get_remote_addr(), ipb, sizeof(ipb)); - SiteThrottledWarning("HTTP/2 session error client_ip=%s session_id=%" PRId64 - " closing a connection, because its stream error rate (%f) exceeded the threshold (%f)", - client_ip, connection_id(), this->connection_state.get_stream_error_rate(), - Http2::stream_error_rate_threshold); - Http2SsnDebug("Preparing for graceful shutdown because of a high stream error rate"); - cause_of_death = Http2SessionCod::HIGH_ERROR_RATE; - this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED, Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM); - } - } +/** + Session State - Waiting all transaction is done (especially for background fill) - if (this->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NOT_INITIATED) { - send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_SHUTDOWN_INIT, this); + No actual read nor write operation. + */ +int +Http2ClientSession::state_aborted(int event, void *edata) +{ + STATE_ENTER(state_aborted, event); + + switch (event) { + case VC_EVENT_ACTIVE_TIMEOUT: + case VC_EVENT_INACTIVITY_TIMEOUT: { + REMEMBER(NO_EVENT, recursion); + Http2SsnDebug("state_closed by %s (%d)", get_vc_event_name(event), event); + _set_state_closed(); + + break; + } + default: + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); + break; } - recursion--; - if (!connection_state.is_recursing() && this->recursion == 0 && kill_me) { - this->free(); + return EVENT_DONE; +} + +/** + Session State - Everything is done + */ +int +Http2ClientSession::state_closed(int event, void *edata) +{ + STATE_ENTER(state_closed, event); + + switch (event) { + default: + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); + break; } - return retval; + + return EVENT_DONE; } +/** + Session Handler + */ int Http2ClientSession::state_read_connection_preface(int event, void *edata) { @@ -397,7 +528,7 @@ Http2ClientSession::state_read_connection_preface(int event, void *edata) if (memcmp(HTTP2_CONNECTION_PREFACE, buf, nbytes) != 0) { Http2SsnDebug("invalid connection preface"); - this->do_io_close(); + handleEvent(VC_EVENT_ERROR); return 0; } @@ -430,6 +561,9 @@ Http2ClientSession::state_read_connection_preface(int event, void *edata) return 0; } +/** + Session Handler + */ int Http2ClientSession::state_start_frame_read(int event, void *edata) { @@ -455,7 +589,7 @@ Http2ClientSession::do_start_frame_read(Http2ErrorCode &ret_error) this->cur_frame_from_early_data = false; if (!http2_parse_frame_header(make_iovec(buf), this->current_hdr)) { Http2SsnDebug("frame header parse failure"); - this->do_io_close(); + handleEvent(VC_EVENT_ERROR); return -1; } @@ -492,6 +626,9 @@ Http2ClientSession::do_start_frame_read(Http2ErrorCode &ret_error) return 0; } +/** + Session Handler + */ int Http2ClientSession::state_complete_frame_read(int event, void *edata) { @@ -542,14 +679,17 @@ Http2ClientSession::do_complete_frame_read() int Http2ClientSession::do_process_frame_read(int event, VIO *vio, bool inside_frame) { + if (!is_state_readable()) { + return EVENT_DONE; + } + if (inside_frame) { do_complete_frame_read(); } - while (this->_read_buffer_reader->read_avail() >= static_cast(HTTP2_FRAME_HEADER_LEN)) { + while (this->_read_buffer_reader->read_avail() >= static_cast(HTTP2_FRAME_HEADER_LEN) && is_state_readable()) { // Cancel reading if there was an error or connection is closed - if (connection_state.tx_error_code.code != static_cast(Http2ErrorCode::HTTP2_ERROR_NO_ERROR) || - connection_state.is_state_closed()) { + if (connection_state.tx_error_code.code != static_cast(Http2ErrorCode::HTTP2_ERROR_NO_ERROR)) { Http2SsnDebug("reading a frame has been canceled (%u)", connection_state.tx_error_code.code); break; } @@ -569,10 +709,7 @@ Http2ClientSession::do_process_frame_read(int event, VIO *vio, bool inside_frame if (err > Http2ErrorCode::HTTP2_ERROR_NO_ERROR || do_start_frame_read(err) < 0) { // send an error if specified. Otherwise, just go away if (err > Http2ErrorCode::HTTP2_ERROR_NO_ERROR) { - if (!this->connection_state.is_state_closed()) { - this->connection_state.send_goaway_frame(this->connection_state.get_latest_stream_id_in(), err); - this->set_half_close_local_flag(true); - } + critical_error(err); } return 0; } @@ -622,7 +759,7 @@ bool Http2ClientSession::_should_do_something_else() { // Do something else every 128 incoming frames if connection state isn't closed - return (this->_n_frame_read & 0x7F) == 0 && !connection_state.is_state_closed(); + return (this->_n_frame_read & 0x7F) == 0 && !this->is_state_closed(); } sockaddr const * @@ -711,3 +848,266 @@ Http2ClientSession::add_url_to_pushed_table(const char *url, int url_len) _h2_pushed_urls->emplace(url); } } + +/** + Handling critical HTTP/2 error cases like PROTOCOL_ERROR + + 1). Send GOAWAY frame + 2). move state_goaway state + 3). indicate TXN(s) VC_EVENT_ERROR + */ +void +Http2ClientSession::critical_error(Http2ErrorCode err) +{ + ink_release_assert(err != Http2ErrorCode::HTTP2_ERROR_NO_ERROR); + + if (session_state == &Http2ClientSession::state_goaway || session_state == &Http2ClientSession::state_closed) { + return; + } + + REMEMBER(NO_EVENT, recursion); + Http2SsnDebug("state_goaway by HTTP/2 error code=%d", (int)err); + _set_state_goaway(err); + + // signal VC_EVENT_ERROR to txns + connection_state.cleanup_streams(VC_EVENT_ERROR); +} + +/** + If there are no stream, add vc in the keep-alive queue + + This may signal VC_EVENT_INACTIVITY_TIMEOUT and start closing VC and SSN + */ +void +Http2ClientSession::add_to_keep_alive_queue() +{ + if (!is_active()) { + return; + } + + if (connection_state.get_client_stream_count() != 0) { + return; + } + + REMEMBER(NO_EVENT, this->recursion); + clear_session_active(); + + if (session_state == &Http2ClientSession::state_aborted) { + // mimicking VC_EVENT_INACTIVITY_TIMEOUT from NetHandler::add_to_keep_alive_queue() + handleEvent(VC_EVENT_INACTIVITY_TIMEOUT); + return; + } + + Http2SsnDebug("keep-alive queue - no streams"); + + UnixNetVConnection *vc = static_cast(_vc); + if (vc && vc->active_timeout_in == 0) { + // With heavy traffic ( - e.g. hitting proxy.config.net.max_connections_in limit), calling add_to_keep_alive_queue() could + // destroy connections in the keep_alive_queue include this vc. In that case, NetHandler raise VC_EVENT_INACTIVITY_TIMEOUT + // event to this ProxySession. + vc->add_to_keep_alive_queue(); + } +} + +bool +Http2ClientSession::is_state_open() const +{ + return session_state == &Http2ClientSession::state_open; +} + +bool +Http2ClientSession::is_state_closed() const +{ + return session_state == &Http2ClientSession::state_closed; +} + +bool +Http2ClientSession::is_state_readable() const +{ + return session_state == &Http2ClientSession::state_open || session_state == &Http2ClientSession::state_graceful_shutdown; +} + +bool +Http2ClientSession::is_state_writable() const +{ + return session_state == &Http2ClientSession::state_open || session_state == &Http2ClientSession::state_graceful_shutdown || + session_state == &Http2ClientSession::state_goaway; +} + +/** + HTTP2_SESSION_EVENT_REENABLE handler for state_open & state_graceful_shutdown + */ +int +Http2ClientSession::_handle_ssn_reenable_event(int event, void *edata) +{ + ink_assert(session_state == &Http2ClientSession::state_open || session_state == &Http2ClientSession::state_graceful_shutdown); + ink_assert(event == HTTP2_SESSION_EVENT_REENABLE); + + if (edata == nullptr) { + return EVENT_DONE; + } + + Event *e = static_cast(edata); + + // VIO will be reenableed in this handler + int res = (this->*session_handler)(VC_EVENT_READ_READY, static_cast(e->cookie)); + + // Clear the event after calling session_handler to not reschedule REENABLE in it + this->_reenable_event = nullptr; + + return res; +} + +/** + VC event handler for state_open & state_graceful_shutdown + */ +int +Http2ClientSession::_handle_vc_event(int event, void *edata) +{ + ink_assert(session_state == &Http2ClientSession::state_open || session_state == &Http2ClientSession::state_graceful_shutdown); + + int res = EVENT_DONE; + + switch (event) { + case VC_EVENT_READ_READY: + case VC_EVENT_READ_COMPLETE: { + ink_assert(edata != nullptr); + ink_assert(edata == read_vio); + + res = (this->*session_handler)(event, edata); + + break; + } + case VC_EVENT_WRITE_READY: + case VC_EVENT_WRITE_COMPLETE: { + ink_assert(edata != nullptr); + ink_assert(edata == write_vio); + + this->connection_state.restart_streams(); + if ((Thread::get_hrtime() >= this->_write_buffer_last_flush + HRTIME_MSECONDS(this->_write_time_threshold))) { + this->flush(); + } + + break; + } + case VC_EVENT_ACTIVE_TIMEOUT: + case VC_EVENT_INACTIVITY_TIMEOUT: { + dying_event = event; + + REMEMBER(event, recursion); + Http2SsnDebug("state_goaway by %s (%d)", get_vc_event_name(event), event); + _set_state_goaway(Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR); + + // signal timeout event to txns + connection_state.cleanup_streams(event); + + break; + } + case VC_EVENT_ERROR: + case VC_EVENT_EOS: { + HTTP2_SET_SESSION_STATE(&Http2ClientSession::state_aborted); + Http2SsnDebug("state_aborted by %s (%d)", get_vc_event_name(event), event); + + dying_event = event; + + // No more read/write op + _vc->do_io_close(); + _vc = nullptr; + + // signal event to txns + connection_state.cleanup_streams(event); + + if (connection_state.get_client_stream_count() == 0) { + REMEMBER(event, recursion); + Http2SsnDebug("state_closed by %s (%d) immediately", get_vc_event_name(event), event); + _set_state_closed(); + } + + break; + } + default: + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + ink_assert(false); + break; + } + + return res; +} + +/** + Check if this session should start Graceful Shutdown or not + */ +bool +Http2ClientSession::_should_start_graceful_shutdown() +{ + if (session_state != &Http2ClientSession::state_open) { + return false; + } + + if (connection_state._graceful_shutdown_enabled) { + return true; + } + + if (this->is_draining()) { + Http2SsnDebug("Preparing for graceful shutdown because of draining state"); + return true; + } + + // For a case many stream errors happened + if (this->connection_state.get_stream_error_rate() > Http2::stream_error_rate_threshold) { + ip_port_text_buffer ipb; + const char *client_ip = ats_ip_ntop(get_remote_addr(), ipb, sizeof(ipb)); + SiteThrottledWarning("HTTP/2 session error client_ip=%s session_id=%" PRId64 + " closing a connection, because its stream error rate (%f) exceeded the threshold (%f)", + client_ip, connection_id(), this->connection_state.get_stream_error_rate(), + Http2::stream_error_rate_threshold); + Http2SsnDebug("Preparing for graceful shutdown because of a high stream error rate"); + cause_of_death = Http2SessionCod::HIGH_ERROR_RATE; + connection_state.shutdown_reason = Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM; + return true; + } + + return false; +} + +/** + Set session_state state_goaway + */ +void +Http2ClientSession::_set_state_goaway(Http2ErrorCode err) +{ + if (session_state == &Http2ClientSession::state_goaway || session_state == &Http2ClientSession::state_closed) { + return; + } + + connection_state.send_goaway_frame(connection_state.get_latest_stream_id_in(), err); + + // Disable read - read is not needed by state_goaway + read_vio->done(); + + // Set nbyte to issue WRITE_COMPLETE event when the GOAWAY frame is sent + int64_t len = _write_buffer_reader->read_avail(); + write_vio->nbytes = write_vio->ndone + len; + + HTTP2_SET_SESSION_STATE(&Http2ClientSession::state_goaway); +} + +/** + Set session_state state_closed + */ +void +Http2ClientSession::_set_state_closed() +{ + if (session_state == &Http2ClientSession::state_closed) { + return; + } + + if (_vc != nullptr) { + _vc->do_io_close(); + _vc = nullptr; + } + + clear_session_active(); + + HTTP2_SET_SESSION_STATE(&Http2ClientSession::state_closed); +} diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h index 73b6dd83d9b..8c50ae280b0 100644 --- a/proxy/http2/Http2ClientSession.h +++ b/proxy/http2/Http2ClientSession.h @@ -33,19 +33,8 @@ #include "tscore/History.h" #include "Milestones.h" -// Name Edata Description -// HTTP2_SESSION_EVENT_INIT Http2ClientSession * HTTP/2 session is born -// HTTP2_SESSION_EVENT_FINI Http2ClientSession * HTTP/2 session is ended -// HTTP2_SESSION_EVENT_RECV Http2Frame * Received a frame -// HTTP2_SESSION_EVENT_XMIT Http2Frame * Send this frame - -#define HTTP2_SESSION_EVENT_INIT (HTTP2_SESSION_EVENTS_START + 1) -#define HTTP2_SESSION_EVENT_FINI (HTTP2_SESSION_EVENTS_START + 2) -#define HTTP2_SESSION_EVENT_RECV (HTTP2_SESSION_EVENTS_START + 3) -#define HTTP2_SESSION_EVENT_XMIT (HTTP2_SESSION_EVENTS_START + 4) -#define HTTP2_SESSION_EVENT_SHUTDOWN_INIT (HTTP2_SESSION_EVENTS_START + 5) -#define HTTP2_SESSION_EVENT_SHUTDOWN_CONT (HTTP2_SESSION_EVENTS_START + 6) -#define HTTP2_SESSION_EVENT_REENABLE (HTTP2_SESSION_EVENTS_START + 7) +#define HTTP2_SESSION_EVENT_GRACEFUL_SHUTDOWN (HTTP2_SESSION_EVENTS_START + 1) +#define HTTP2_SESSION_EVENT_REENABLE (HTTP2_SESSION_EVENTS_START + 2) enum class Http2SessionCod : int { NOT_PROVIDED, @@ -61,6 +50,35 @@ enum class Http2SsnMilestone { size_t const HTTP2_HEADER_BUFFER_SIZE_INDEX = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX; /** + @startuml + title ProxySession Continuation Handler + hide empty description + + [*] --> main_event_handler : start() + main_event_Handler --> state_api_callout : do_api_callout()/handle_api_return() + + @enduml + + @startuml + title HTTP/2 Session States + hide empty description + + [*] --> state_open : start() + state_open --> state_open : VC_EVENT_READ_READY\lVC_EVENT_READ_COMPLETE\lVC_EVENT_WRITE_READY\lVC_EVENT_WRITE_COMPLETE + + state_open --> state_aborted : VC_EVENT_EOS\lVC_EVENT_ERROR\lrecv GOAWAY frame with error code + state_aborted --> state_closed : all transaction is done + + state_open --> state_graceful_shutdown : graceful shutdown + state_graceful_shutdown --> state_closed : all transaction is done + + state_open --> state_goaway : VC_EVENT_ACTIVE_TIMEOUT\lVC_EVENT_INACTIVITY_TIMEOUT\lsend GOAWAY frame with error code + state_goaway --> state_closed : VC_EVENT_WRITE_COMPLETE\l(completed sending GOAWAY frame) + + state_closed --> [*] : destroy() + + @enduml + @startuml title HTTP/2 Session Handler - state of reading HTTP/2 frame hide empty description @@ -77,6 +95,7 @@ class Http2ClientSession : public ProxySession { public: using super = ProxySession; ///< Parent type. + using SessionState = int (Http2ClientSession::*)(int, void *); using SessionHandler = int (Http2ClientSession::*)(int, void *); Http2ClientSession(); @@ -85,7 +104,11 @@ class Http2ClientSession : public ProxySession // Methods // Implement VConnection interface + VIO *do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) override; + VIO *do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *reader, bool owner) override; void do_io_close(int lerrno = -1) override; + void do_io_shutdown(ShutdownHowTo_t howto) override; + void reenable(VIO *vio) override; // Implement ProxySession interface void new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader) override; @@ -111,13 +134,7 @@ class Http2ClientSession : public ProxySession void increment_current_active_connections_stat() override; void decrement_current_active_connections_stat() override; - void set_dying_event(int event); - int get_dying_event() const; - bool ready_to_free() const; - bool is_recursing() const; - void set_half_close_local_flag(bool flag); - bool get_half_close_local_flag() const; - bool is_url_pushed(const char *url, int url_len); + bool is_url_pushed(const char *url, int url_len) const; void add_url_to_pushed_table(const char *url, int url_len); // Record history from Http2ConnectionState @@ -125,6 +142,14 @@ class Http2ClientSession : public ProxySession int64_t write_avail(); + void critical_error(Http2ErrorCode err); + void add_to_keep_alive_queue(); + + bool is_state_open() const; + bool is_state_closed() const; + bool is_state_readable() const; + bool is_state_writable() const; + // noncopyable Http2ClientSession(Http2ClientSession &) = delete; Http2ClientSession &operator=(const Http2ClientSession &) = delete; @@ -134,9 +159,17 @@ class Http2ClientSession : public ProxySession Http2ConnectionState connection_state; private: - int main_event_handler(int, void *); + // Continuation Handler + int main_event_handler(int event, void *edata); - // SessionHandler(s) - state of reading frame + // Session States + int state_open(int event, void *edata); + int state_aborted(int event, void *edata); + int state_goaway(int event, void *edata); + int state_graceful_shutdown(int event, void *edata); + int state_closed(int event, void *edata); + + // Session Handler - state of reading frame int state_read_connection_preface(int, void *); int state_start_frame_read(int, void *); int state_complete_frame_read(int, void *); @@ -148,11 +181,20 @@ class Http2ClientSession : public ProxySession int do_complete_frame_read(); bool _should_do_something_else(); + bool _should_start_graceful_shutdown(); + + void _set_state_goaway(Http2ErrorCode err); + void _set_state_closed(); + + int _handle_ssn_reenable_event(int event, void *edata); + int _handle_vc_event(int event, void *edata); //////// // Variables + SessionState session_state = nullptr; SessionHandler session_handler = nullptr; + VIO *read_vio = nullptr; MIOBuffer *read_buffer = nullptr; IOBufferReader *_read_buffer_reader = nullptr; @@ -172,15 +214,14 @@ class Http2ClientSession : public ProxySession Milestones(Http2SsnMilestone::LAST_ENTRY)> _milestones; int dying_event = 0; - bool kill_me = false; Http2SessionCod cause_of_death = Http2SessionCod::NOT_PROVIDED; - bool half_close_local = false; int recursion = 0; std::unordered_set *_h2_pushed_urls = nullptr; - Event *_reenable_event = nullptr; - int _n_frame_read = 0; + Event *_graceful_shutdown_event = nullptr; + Event *_reenable_event = nullptr; + int _n_frame_read = 0; uint32_t _pending_sending_data_size = 0; @@ -194,37 +235,7 @@ extern ClassAllocator http2ClientSessionAllocator; // INLINE inline bool -Http2ClientSession::ready_to_free() const -{ - return kill_me; -} - -inline void -Http2ClientSession::set_dying_event(int event) -{ - dying_event = event; -} - -inline int -Http2ClientSession::get_dying_event() const -{ - return dying_event; -} - -inline bool -Http2ClientSession::is_recursing() const -{ - return recursion > 0; -} - -inline bool -Http2ClientSession::get_half_close_local_flag() const -{ - return half_close_local; -} - -inline bool -Http2ClientSession::is_url_pushed(const char *url, int url_len) +Http2ClientSession::is_url_pushed(const char *url, int url_len) const { if (_h2_pushed_urls == nullptr) { return false; diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 66855433729..949b39eeee9 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/proxy/http2/Http2ConnectionState.cc @@ -88,10 +88,6 @@ rcv_data_frame(Http2ConnectionState &cstate, const Http2Frame &frame) Http2StreamDebug(cstate.ua_session, id, "Received DATA frame"); - if (cstate.get_zombie_event()) { - Warning("Data frame for zombied session %" PRId64, cstate.ua_session->connection_id()); - } - // If a DATA frame is received whose stream identifier field is 0x0, the // recipient MUST // respond with a connection error of type PROTOCOL_ERROR. @@ -418,10 +414,6 @@ rcv_priority_frame(Http2ConnectionState &cstate, const Http2Frame &frame) Http2StreamDebug(cstate.ua_session, stream_id, "Received PRIORITY frame"); - if (cstate.get_zombie_event()) { - Warning("Priority frame for zombied session %" PRId64, cstate.ua_session->connection_id()); - } - // If a PRIORITY frame is received with a stream identifier of 0x0, the // recipient MUST respond with a connection error of type PROTOCOL_ERROR. if (stream_id == 0) { @@ -565,10 +557,6 @@ rcv_settings_frame(Http2ConnectionState &cstate, const Http2Frame &frame) Http2StreamDebug(cstate.ua_session, stream_id, "Received SETTINGS frame"); - if (cstate.get_zombie_event()) { - Warning("Setting frame for zombied session %" PRId64, cstate.ua_session->connection_id()); - } - // Update SETTIGNS frame count per minute cstate.increment_received_settings_frame_count(); // Close this connection if its SETTINGS frame count exceeds a limit @@ -687,8 +675,6 @@ rcv_ping_frame(Http2ConnectionState &cstate, const Http2Frame &frame) Http2StreamDebug(cstate.ua_session, stream_id, "Received PING frame"); - cstate.schedule_zombie_event(); - // If a PING frame is received with a stream identifier field value other // than 0x0, the recipient MUST respond with a connection error of type // PROTOCOL_ERROR. @@ -757,7 +743,8 @@ rcv_goaway_frame(Http2ConnectionState &cstate, const Http2Frame &frame) static_cast(goaway.error_code)); cstate.rx_error_code = {ProxyErrorClass::SSN, static_cast(goaway.error_code)}; - cstate.ua_session->do_io_close(); + // TODO: support remote peer started gracefull shutdown + cstate.ua_session->handleEvent(VC_EVENT_ERROR); return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); } @@ -1034,10 +1021,7 @@ Http2ConnectionSettings::indexof(Http2SettingsIdentifier id) //////// // Http2ConnectionState // -Http2ConnectionState::Http2ConnectionState() : stream_list() -{ - SET_HANDLER(&Http2ConnectionState::main_event_handler); -} +Http2ConnectionState::Http2ConnectionState() : stream_list() {} void Http2ConnectionState::init(Http2ClientSession *ssn) @@ -1051,7 +1035,7 @@ Http2ConnectionState::init(Http2ClientSession *ssn) dependency_tree = new DependencyTree(Http2::max_concurrent_streams_in); } - _cop = ActivityCop(this->mutex, &stream_list, 1); + _cop = ActivityCop(ssn->mutex, &stream_list, 1); _cop.start(); } @@ -1068,7 +1052,7 @@ Http2ConnectionState::init(Http2ClientSession *ssn) void Http2ConnectionState::send_connection_preface() { - REMEMBER(NO_EVENT, this->recursion) + REMEMBER(NO_EVENT, 0); Http2ConnectionSettings configured_settings; configured_settings.settings_from_configs(); @@ -1081,45 +1065,26 @@ Http2ConnectionState::send_connection_preface() } } +// TODO: move to the destructor void Http2ConnectionState::destroy() { - if (in_destroy) { - schedule_zombie_event(); - return; - } - in_destroy = true; - _cop.stop(); - if (shutdown_cont_event) { - shutdown_cont_event->cancel(); - shutdown_cont_event = nullptr; - } - cleanup_streams(); - delete local_hpack_handle; local_hpack_handle = nullptr; + delete remote_hpack_handle; remote_hpack_handle = nullptr; - delete dependency_tree; - dependency_tree = nullptr; - this->ua_session = nullptr; - if (fini_event) { - fini_event->cancel(); - } - if (zombie_event) { - zombie_event->cancel(); - } - // release the mutex after the events are cancelled and sessions are destroyed. - mutex = nullptr; // magic happens - assigning to nullptr frees the ProxyMutex + delete dependency_tree; + dependency_tree = nullptr; } void Http2ConnectionState::rcv_frame(const Http2Frame *frame) { - REMEMBER(NO_EVENT, this->recursion); + REMEMBER(NO_EVENT, 0); const Http2StreamId stream_id = frame->header().streamid; Http2Error error; @@ -1163,13 +1128,7 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame) Error("HTTP/2 connection error code=0x%02x client_ip=%s session_id=%" PRId64 " stream_id=%u %s", static_cast(error.code), client_ip, ua_session->connection_id(), stream_id, error.msg); } - this->send_goaway_frame(this->latest_streamid_in, error.code); - this->ua_session->set_half_close_local_flag(true); - if (fini_event == nullptr) { - fini_event = this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI); - } - - // The streams will be cleaned up by the HTTP2_SESSION_EVENT_FINI event + ua_session->critical_error(error.code); // The Http2ClientSession will shutdown because connection_state.is_state_closed() will be true } else if (error.cls == Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM) { if (error.msg) { @@ -1181,104 +1140,6 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame) } } -int -Http2ConnectionState::main_event_handler(int event, void *edata) -{ - if (edata == zombie_event) { - // zombie session is still around. Assert - ink_release_assert(zombie_event == nullptr); - } else if (edata == fini_event) { - fini_event = nullptr; - } - ++recursion; - switch (event) { - // Finalize HTTP/2 Connection - case HTTP2_SESSION_EVENT_FINI: { - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - REMEMBER(event, this->recursion); - - ink_assert(this->fini_received == false); - this->fini_received = true; - cleanup_streams(); - release_stream(); - SET_HANDLER(&Http2ConnectionState::state_closed); - } break; - - case HTTP2_SESSION_EVENT_XMIT: { - REMEMBER(event, this->recursion); - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - send_data_frames_depends_on_priority(); - _scheduled = false; - } break; - - // Initiate a graceful shutdown - case HTTP2_SESSION_EVENT_SHUTDOWN_INIT: { - REMEMBER(event, this->recursion); - ink_assert(shutdown_state == HTTP2_SHUTDOWN_NOT_INITIATED); - shutdown_state = HTTP2_SHUTDOWN_INITIATED; - // [RFC 7540] 6.8. GOAWAY - // A server that is attempting to gracefully shut down a - // connection SHOULD send an initial GOAWAY frame with the last stream - // identifier set to 2^31-1 and a NO_ERROR code. - send_goaway_frame(INT32_MAX, Http2ErrorCode::HTTP2_ERROR_NO_ERROR); - // After allowing time for any in-flight stream creation (at least one round-trip time), - shutdown_cont_event = this_ethread()->schedule_in((Continuation *)this, HRTIME_SECONDS(2), HTTP2_SESSION_EVENT_SHUTDOWN_CONT); - } break; - - // Continue a graceful shutdown - case HTTP2_SESSION_EVENT_SHUTDOWN_CONT: { - REMEMBER(event, this->recursion); - ink_assert(shutdown_state == HTTP2_SHUTDOWN_INITIATED); - shutdown_cont_event = nullptr; - shutdown_state = HTTP2_SHUTDOWN_IN_PROGRESS; - // [RFC 7540] 6.8. GOAWAY - // ..., the server can send another GOAWAY frame with an updated last stream identifier - if (shutdown_reason == Http2ErrorCode::HTTP2_ERROR_MAX) { - shutdown_reason = Http2ErrorCode::HTTP2_ERROR_NO_ERROR; - } - send_goaway_frame(latest_streamid_in, shutdown_reason); - // Stop creating new streams - SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread()); - this->ua_session->set_half_close_local_flag(true); - } break; - - default: - Http2ConDebug(ua_session, "unexpected event=%d edata=%p", event, edata); - ink_release_assert(0); - break; - } - - --recursion; - if (recursion == 0 && ua_session && !ua_session->is_recursing()) { - if (this->ua_session->ready_to_free()) { - MUTEX_TRY_LOCK(lock, this->ua_session->mutex, this_ethread()); - if (lock.is_locked()) { - this->ua_session->free(); - // After the free, the Http2ConnectionState object is also freed. - // The Http2ConnectionState object is allocated within the Http2ClientSession object - } - } - } - - return 0; -} - -int -Http2ConnectionState::state_closed(int event, void *edata) -{ - REMEMBER(event, this->recursion); - - if (edata == zombie_event) { - // Zombie session is still around. Assert! - ink_release_assert(zombie_event == nullptr); - } else if (edata == fini_event) { - fini_event = nullptr; - } else if (edata == shutdown_cont_event) { - shutdown_cont_event = nullptr; - } - return 0; -} - Http2Stream * Http2ConnectionState::create_stream(Http2StreamId new_id, Http2Error &error) { @@ -1289,8 +1150,7 @@ Http2ConnectionState::create_stream(Http2StreamId new_id, Http2Error &error) return nullptr; } - // In half_close state, TS doesn't create new stream. Because GOAWAY frame is sent to client - if (ua_session->get_half_close_local_flag()) { + if (!ua_session->is_state_open()) { error = Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM, "refused to create new stream, because ua_session is in half_close state"); return nullptr; @@ -1353,11 +1213,6 @@ Http2ConnectionState::create_stream(Http2StreamId new_id, Http2Error &error) } ++total_client_streams_count; - if (zombie_event != nullptr) { - zombie_event->cancel(); - zombie_event = nullptr; - } - new_stream->mutex = new_ProxyMutex(); new_stream->is_first_transaction_flag = get_stream_requests() == 0; increment_stream_requests(); @@ -1380,6 +1235,11 @@ Http2ConnectionState::find_stream(Http2StreamId id) const void Http2ConnectionState::restart_streams() { + if (Http2::stream_priority_enabled) { + send_data_frames_depends_on_priority(); + return; + } + Http2Stream *s = stream_list.head; if (s) { Http2Stream *end = s; @@ -1445,9 +1305,24 @@ Http2ConnectionState::restart_receiving(Http2Stream *stream) this->send_window_update_frame(stream->get_id(), diff_size); } +/** + Signal @event to TXN(s) + */ void -Http2ConnectionState::cleanup_streams() +Http2ConnectionState::cleanup_streams(int event) { + switch (event) { + case VC_EVENT_ACTIVE_TIMEOUT: + case VC_EVENT_INACTIVITY_TIMEOUT: + case VC_EVENT_ERROR: + case VC_EVENT_EOS: + // do nothing - expected events + break; + default: + ink_abort("unexpected event %d", event); + break; + } + Http2Stream *s = stream_list.head; while (s) { Http2Stream *next = static_cast(s->link.next); @@ -1457,26 +1332,19 @@ Http2ConnectionState::cleanup_streams() if (this->tx_error_code.cls != ProxyErrorClass::NONE) { s->set_tx_error_code(this->tx_error_code); } - s->initiating_close(); + { + SCOPED_MUTEX_LOCK(lock, s->mutex, this_ethread()); + s->handleEvent(event); + } ink_assert(s != next); s = next; } - - if (!is_state_closed()) { - SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread()); - - UnixNetVConnection *vc = static_cast(ua_session->get_netvc()); - if (vc && vc->active_timeout_in == 0) { - vc->add_to_keep_alive_queue(); - } - } } bool Http2ConnectionState::delete_stream(Http2Stream *stream) { ink_assert(nullptr != stream); - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); // If stream has already been removed from the list, just go on if (!stream_list.in(stream)) { @@ -1484,7 +1352,7 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) } Http2StreamDebug(ua_session, stream->get_id(), "Delete stream"); - REMEMBER(NO_EVENT, this->recursion); + REMEMBER(NO_EVENT, 0); if (Http2::stream_priority_enabled) { Http2DependencyTree::Node *node = stream->priority_node; @@ -1503,7 +1371,7 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) stream->priority_node = nullptr; } - if (stream->get_state() != Http2StreamState::HTTP2_STREAM_STATE_CLOSED) { + if (ua_session->is_state_writable() && stream->get_state() != Http2StreamState::HTTP2_STREAM_STATE_CLOSED) { send_rst_stream_frame(stream->get_id(), Http2ErrorCode::HTTP2_ERROR_NO_ERROR); } @@ -1523,45 +1391,6 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) return true; } -void -Http2ConnectionState::release_stream() -{ - REMEMBER(NO_EVENT, this->recursion) - - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - if (this->ua_session) { - ink_assert(this->mutex == ua_session->mutex); - - if (total_client_streams_count == 0) { - if (fini_received) { - ua_session->clear_session_active(); - - // We were shutting down, go ahead and terminate the session - // this is a member of Http2ConnectionState and will be freed - // when ua_session is destroyed - ua_session->destroy(); - - // Can't do this because we just destroyed right here ^, - // or we can use a local variable to do it. - // ua_session = nullptr; - } else if (ua_session->is_active()) { - // If the number of clients is 0, HTTP2_SESSION_EVENT_FINI is not received or sent, and ua_session is active, - // then mark the connection as inactive - ua_session->clear_session_active(); - UnixNetVConnection *vc = static_cast(ua_session->get_netvc()); - if (vc && vc->active_timeout_in == 0) { - // With heavy traffic, ua_session could be destroyed. Do not touch ua_session after this. - vc->add_to_keep_alive_queue(); - } - } else { - schedule_zombie_event(); - } - } else if (fini_received) { - schedule_zombie_event(); - } - } -} - void Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size) { @@ -1572,67 +1401,52 @@ Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size) } } -void -Http2ConnectionState::schedule_stream(Http2Stream *stream) -{ - Http2StreamDebug(ua_session, stream->get_id(), "Scheduled"); - - Http2DependencyTree::Node *node = stream->priority_node; - ink_release_assert(node != nullptr); - - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - dependency_tree->activate(node); - - if (!_scheduled) { - _scheduled = true; - - SET_HANDLER(&Http2ConnectionState::main_event_handler); - this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_XMIT); - } -} - void Http2ConnectionState::send_data_frames_depends_on_priority() { Http2DependencyTree::Node *node = dependency_tree->top(); + while (node && _client_rwnd > 0) { + Http2Stream *stream = static_cast(node->t); + ink_release_assert(stream != nullptr); - // No node to send or no connection level window left - if (node == nullptr || _client_rwnd <= 0) { - return; - } + if (stream->is_closed() || stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_CLOSED) { + dependency_tree->deactivate(node, 0); + node = dependency_tree->top(); + continue; + } - Http2Stream *stream = static_cast(node->t); - ink_release_assert(stream != nullptr); - Http2StreamDebug(ua_session, stream->get_id(), "top node, point=%d", node->point); + Http2StreamDebug(ua_session, stream->get_id(), "top node, point=%d", node->point); - size_t len = 0; - Http2SendDataFrameResult result = send_a_data_frame(stream, len); + size_t len = 0; + Http2SendDataFrameResult result = send_a_data_frame(stream, len); - switch (result) { - case Http2SendDataFrameResult::NO_ERROR: { - // No response body to send - if (len == 0 && !stream->is_write_vio_done()) { - dependency_tree->deactivate(node, len); - } else { - dependency_tree->update(node, len); + switch (result) { + case Http2SendDataFrameResult::NO_ERROR: { + // No response body to send + if (len == 0 && !stream->is_write_vio_done()) { + dependency_tree->deactivate(node, len); + } else { + dependency_tree->update(node, len); - SCOPED_MUTEX_LOCK(stream_lock, stream->mutex, this_ethread()); - stream->signal_write_event(true); + SCOPED_MUTEX_LOCK(stream_lock, stream->mutex, this_ethread()); + stream->signal_write_event(true); + } + break; } - break; - } - case Http2SendDataFrameResult::DONE: { - dependency_tree->deactivate(node, len); - stream->initiating_close(); - break; - } - default: - // When no stream level window left, deactivate node once and wait window_update frame - dependency_tree->deactivate(node, len); - break; + case Http2SendDataFrameResult::DONE: { + dependency_tree->deactivate(node, len); + stream->initiating_close(); + break; + } + default: + // When no stream level window left, deactivate node once and wait window_update frame + dependency_tree->deactivate(node, len); + break; + } + + node = dependency_tree->top(); } - this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_XMIT); return; } @@ -1785,11 +1599,7 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream) // Change stream state if (!stream->change_state(HTTP2_FRAME_TYPE_HEADERS, flags)) { - this->send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR); - this->ua_session->set_half_close_local_flag(true); - if (fini_event == nullptr) { - fini_event = this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI); - } + ua_session->critical_error(Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR); return; } @@ -1924,7 +1734,7 @@ Http2ConnectionState::send_push_promise_frame(Http2Stream *stream, URL &url, con void Http2ConnectionState::send_rst_stream_frame(Http2StreamId id, Http2ErrorCode ec) { - Http2StreamDebug(ua_session, id, "Send RST_STREAM frame"); + Http2StreamDebug(ua_session, id, "Send RST_STREAM frame ec=%d", static_cast(ec)); if (ec != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) { HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_STREAM_ERRORS_COUNT, this_ethread()); @@ -1936,12 +1746,7 @@ Http2ConnectionState::send_rst_stream_frame(Http2StreamId id, Http2ErrorCode ec) if (stream != nullptr) { stream->set_tx_error_code({ProxyErrorClass::TXN, static_cast(ec)}); if (!stream->change_state(HTTP2_FRAME_TYPE_RST_STREAM, 0)) { - this->send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR); - this->ua_session->set_half_close_local_flag(true); - if (fini_event == nullptr) { - fini_event = this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI); - } - + ua_session->critical_error(Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR); return; } } @@ -1995,7 +1800,7 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec) { ink_assert(this->ua_session != nullptr); - Http2ConDebug(ua_session, "Send GOAWAY frame, last_stream_id: %d", id); + Http2ConDebug(ua_session, "Send GOAWAY frame, last_stream_id=%d ec=%d", id, static_cast(ec)); if (ec != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) { HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CONNECTION_ERRORS_COUNT, this_ethread()); diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index 3c55bdfaa3b..0fc1acadda2 100644 --- a/proxy/http2/Http2ConnectionState.h +++ b/proxy/http2/Http2ConnectionState.h @@ -45,8 +45,6 @@ enum class Http2SendDataFrameResult { DONE, }; -enum Http2ShutdownState { HTTP2_SHUTDOWN_NONE, HTTP2_SHUTDOWN_NOT_INITIATED, HTTP2_SHUTDOWN_INITIATED, HTTP2_SHUTDOWN_IN_PROGRESS }; - class Http2ConnectionSettings { public: @@ -63,21 +61,24 @@ class Http2ConnectionSettings unsigned settings[HTTP2_SETTINGS_MAX - 1]; }; -// Http2ConnectionState -// -// Capture the semantics of a HTTP/2 connection. The client session captures the -// frame layer, and the -// connection state captures the connection-wide state. +/** + Http2ConnectionState -class Http2ConnectionState : public Continuation + Capture the semantics of a HTTP/2 connection +*/ +class Http2ConnectionState { public: + friend class Http2ClientSession; + Http2ConnectionState(); // noncopyable Http2ConnectionState(const Http2ConnectionState &) = delete; Http2ConnectionState &operator=(const Http2ConnectionState &) = delete; + /////////////////// + // Variables ProxyError rx_error_code; ProxyError tx_error_code; Http2ClientSession *ua_session = nullptr; @@ -90,6 +91,8 @@ class Http2ConnectionState : public Continuation Http2ConnectionSettings server_settings; Http2ConnectionSettings client_settings; + /////////////////// + // Methods void init(Http2ClientSession *ssn); void send_connection_preface(); void destroy(); @@ -104,8 +107,7 @@ class Http2ConnectionState : public Continuation Http2Stream *find_stream(Http2StreamId id) const; void restart_streams(); bool delete_stream(Http2Stream *stream); - void release_stream(); - void cleanup_streams(); + void cleanup_streams(int event); void restart_receiving(Http2Stream *stream); void update_initial_rwnd(Http2WindowSize new_size); @@ -122,10 +124,8 @@ class Http2ConnectionState : public Continuation uint32_t get_client_stream_count() const; void decrement_stream_count(); double get_stream_error_rate() const; - Http2ErrorCode get_shutdown_reason() const; // HTTP/2 frame sender - void schedule_stream(Http2Stream *stream); void send_data_frames_depends_on_priority(); void send_data_frames(Http2Stream *stream); Http2SendDataFrameResult send_a_data_frame(Http2Stream *stream, size_t &payload_length); @@ -137,16 +137,8 @@ class Http2ConnectionState : public Continuation void send_goaway_frame(Http2StreamId id, Http2ErrorCode ec); void send_window_update_frame(Http2StreamId id, uint32_t size); - bool is_state_closed() const; - bool is_recursing() const; bool is_valid_streamid(Http2StreamId id) const; - Http2ShutdownState get_shutdown_state() const; - void set_shutdown_state(Http2ShutdownState state, Http2ErrorCode reason = Http2ErrorCode::HTTP2_ERROR_NO_ERROR); - - Event *get_zombie_event(); - void schedule_zombie_event(); - void increment_received_settings_count(uint32_t count); uint32_t get_received_settings_count(); void increment_received_settings_frame_count(); @@ -163,6 +155,8 @@ class Http2ConnectionState : public Continuation Http2ErrorCode increment_server_rwnd(size_t amount); Http2ErrorCode decrement_server_rwnd(size_t amount); + void enable_graceful_shutdown(); + private: unsigned _adjust_concurrent_stream(); @@ -209,15 +203,9 @@ class Http2ConnectionState : public Continuation // "If the END_HEADERS bit is not set, this frame MUST be followed by // another CONTINUATION frame." Http2StreamId continued_stream_id = 0; - bool _scheduled = false; - bool fini_received = false; - bool in_destroy = false; - int recursion = 0; - Http2ShutdownState shutdown_state = HTTP2_SHUTDOWN_NONE; Http2ErrorCode shutdown_reason = Http2ErrorCode::HTTP2_ERROR_MAX; - Event *shutdown_cont_event = nullptr; - Event *fini_event = nullptr; - Event *zombie_event = nullptr; + + bool _graceful_shutdown_enabled = false; }; /////////////////////////////////////////////// @@ -290,24 +278,6 @@ Http2ConnectionState::get_stream_error_rate() const } } -inline Http2ErrorCode -Http2ConnectionState::get_shutdown_reason() const -{ - return shutdown_reason; -} - -inline bool -Http2ConnectionState::is_state_closed() const -{ - return ua_session == nullptr || fini_received; -} - -inline bool -Http2ConnectionState::is_recursing() const -{ - return recursion > 0; -} - inline bool Http2ConnectionState::is_valid_streamid(Http2StreamId id) const { @@ -318,32 +288,8 @@ Http2ConnectionState::is_valid_streamid(Http2StreamId id) const } } -inline Http2ShutdownState -Http2ConnectionState::get_shutdown_state() const -{ - return shutdown_state; -} - -inline void -Http2ConnectionState::set_shutdown_state(Http2ShutdownState state, Http2ErrorCode reason) -{ - shutdown_state = state; - shutdown_reason = reason; -} - -inline Event * -Http2ConnectionState::get_zombie_event() -{ - return zombie_event; -} - inline void -Http2ConnectionState::schedule_zombie_event() +Http2ConnectionState::enable_graceful_shutdown() { - if (Http2::zombie_timeout_in) { // If we have zombie debugging enabled - if (zombie_event) { - zombie_event->cancel(); - } - zombie_event = this_ethread()->schedule_in(this, HRTIME_SECONDS(Http2::zombie_timeout_in)); - } + _graceful_shutdown_enabled = true; } diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 1990b15e35d..1efbe10ba0a 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -82,13 +82,10 @@ Http2Stream::~Http2Stream() // Make sure the stream is removed from the stream list and priority tree // 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(); - - // Do not access `_proxy_ssn` in below. It might be freed by `release_stream`. + // Do not access `_proxy_ssn` in below. It might be freed. + h2_proxy_ssn->add_to_keep_alive_queue(); } // Clean up the write VIO in case of inactivity timeout @@ -149,22 +146,26 @@ Http2Stream::main_event_handler(int event, void *edata) } ink_release_assert(this->_thread == this_ethread()); + Http2StreamDebug("%s (%d)", get_vc_event_name(event), event); + Event *e = static_cast(edata); reentrancy_count++; - if (e == _read_vio_event) { - _read_vio_event = nullptr; - this->signal_read_event(e->callback_event); - return 0; - } else if (e == _write_vio_event) { - _write_vio_event = nullptr; - this->signal_write_event(e->callback_event); - return 0; - } else if (e == cross_thread_event) { - cross_thread_event = nullptr; - } else if (e == read_event) { - read_event = nullptr; - } else if (e == write_event) { - write_event = nullptr; + if (e != nullptr) { + if (e == _read_vio_event) { + _read_vio_event = nullptr; + this->signal_read_event(e->callback_event); + return 0; + } else if (e == _write_vio_event) { + _write_vio_event = nullptr; + this->signal_write_event(e->callback_event); + return 0; + } else if (e == cross_thread_event) { + cross_thread_event = nullptr; + } else if (e == read_event) { + read_event = nullptr; + } else if (e == write_event) { + write_event = nullptr; + } } switch (event) { @@ -179,7 +180,7 @@ Http2Stream::main_event_handler(int event, void *edata) case VC_EVENT_WRITE_READY: case VC_EVENT_WRITE_COMPLETE: _timeout.update_inactivity(); - if (e->cookie == &write_vio) { + if (e && e->cookie == &write_vio) { if (write_vio.mutex && write_vio.cont && this->_sm) { this->signal_write_event(event); } @@ -190,7 +191,7 @@ Http2Stream::main_event_handler(int event, void *edata) case VC_EVENT_READ_COMPLETE: case VC_EVENT_READ_READY: _timeout.update_inactivity(); - if (e->cookie == &read_vio) { + if (e && e->cookie == &read_vio) { if (read_vio.mutex && read_vio.cont && this->_sm) { signal_read_event(event); } @@ -198,14 +199,26 @@ Http2Stream::main_event_handler(int event, void *edata) this->update_read_request(true); } break; + case VC_EVENT_ERROR: case VC_EVENT_EOS: - if (e->cookie == &read_vio) { + if (e && e->cookie == &read_vio) { SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread()); - read_vio.cont->handleEvent(VC_EVENT_EOS, &read_vio); - } else if (e->cookie == &write_vio) { + read_vio.cont->handleEvent(event, &read_vio); + } else if (e && e->cookie == &write_vio) { SCOPED_MUTEX_LOCK(lock, write_vio.mutex, this_ethread()); - write_vio.cont->handleEvent(VC_EVENT_EOS, &write_vio); + write_vio.cont->handleEvent(event, &write_vio); + } + + // This assuming stream on inbound side + // TODO: make appropriate state + if (recv_end_stream && write_vio.cont && write_vio.op == VIO::WRITE && write_vio.nbytes != 0) { + SCOPED_MUTEX_LOCK(lock, write_vio.cont->mutex, this_ethread()); + write_vio.cont->handleEvent(event, &write_vio); + } else if (read_vio.cont && read_vio.op == VIO::READ) { + SCOPED_MUTEX_LOCK(lock, read_vio.cont->mutex, this_ethread()); + read_vio.cont->handleEvent(event, &read_vio); } + break; } reentrancy_count--; @@ -432,6 +445,12 @@ Http2Stream::do_io_close(int /* flags */) // by the time this is called from transaction_done. closed = true; + // Disable read/write + read_vio.op = VIO::NONE; + read_vio.cont = nullptr; + write_vio.op = VIO::NONE; + write_vio.cont = nullptr; + if (_proxy_ssn && this->is_client_state_writeable()) { // Make sure any trailing end of stream frames are sent // We will be removed at send_data_frames or closing connection phase @@ -459,7 +478,7 @@ Http2Stream::transaction_done() if (!closed) { do_io_close(); // Make sure we've been closed. If we didn't close the _proxy_ssn session better still be open } - ink_release_assert(closed || !static_cast(_proxy_ssn)->connection_state.is_state_closed()); + ink_release_assert(closed); _sm = nullptr; if (closed) { @@ -660,8 +679,8 @@ Http2Stream::update_write_request(bool call_update) const char *value = field->value_get(&len); if (memcmp(HTTP_VALUE_CLOSE, value, HTTP_LEN_CLOSE) == 0) { SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->mutex, this_ethread()); - if (h2_proxy_ssn->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NONE) { - h2_proxy_ssn->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED, Http2ErrorCode::HTTP2_ERROR_NO_ERROR); + if (h2_proxy_ssn->is_state_open()) { + h2_proxy_ssn->connection_state.enable_graceful_shutdown(); } } } @@ -784,7 +803,8 @@ Http2Stream::send_response_body(bool call_update) if (Http2::stream_priority_enabled) { SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->mutex, this_ethread()); - h2_proxy_ssn->connection_state.schedule_stream(this); + h2_proxy_ssn->connection_state.dependency_tree->activate(priority_node); + h2_proxy_ssn->write_reenable(); // signal_write_event() will be called from `Http2ConnectionState::send_data_frames_depends_on_priority()` // when write_vio is consumed } else { diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index ab980567acd..aec84b0a081 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -8223,7 +8223,7 @@ TSHttpTxnServerPush(TSHttpTxn txnp, const char *url, int url_len) Http2ClientSession *ua_session = static_cast(stream->get_proxy_ssn()); SCOPED_MUTEX_LOCK(lock, ua_session->mutex, this_ethread()); - if (ua_session->connection_state.is_state_closed() || ua_session->is_url_pushed(url, url_len)) { + if (ua_session->is_state_closed() || ua_session->is_url_pushed(url, url_len)) { url_obj.destroy(); return TS_ERROR; } From 98a8b73b1ab2b7acaa8bfa7a5b910030463d0255 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 6 Jul 2021 08:21:42 +0900 Subject: [PATCH 2/4] Fix use-after-free on Http2Stream --- proxy/http2/Http2Stream.cc | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 1efbe10ba0a..13be5afb110 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -201,22 +201,24 @@ Http2Stream::main_event_handler(int event, void *edata) break; case VC_EVENT_ERROR: case VC_EVENT_EOS: - if (e && e->cookie == &read_vio) { - SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread()); - read_vio.cont->handleEvent(event, &read_vio); - } else if (e && e->cookie == &write_vio) { - SCOPED_MUTEX_LOCK(lock, write_vio.mutex, this_ethread()); - write_vio.cont->handleEvent(event, &write_vio); - } - - // This assuming stream on inbound side - // TODO: make appropriate state - if (recv_end_stream && write_vio.cont && write_vio.op == VIO::WRITE && write_vio.nbytes != 0) { - SCOPED_MUTEX_LOCK(lock, write_vio.cont->mutex, this_ethread()); - write_vio.cont->handleEvent(event, &write_vio); - } else if (read_vio.cont && read_vio.op == VIO::READ) { - SCOPED_MUTEX_LOCK(lock, read_vio.cont->mutex, this_ethread()); - read_vio.cont->handleEvent(event, &read_vio); + if (e != nullptr) { + if (e->cookie == &read_vio) { + SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread()); + read_vio.cont->handleEvent(event, &read_vio); + } else if (e->cookie == &write_vio) { + SCOPED_MUTEX_LOCK(lock, write_vio.mutex, this_ethread()); + write_vio.cont->handleEvent(event, &write_vio); + } + } else { + // This assuming stream on inbound side + // TODO: make appropriate state + if (recv_end_stream && write_vio.cont && write_vio.op == VIO::WRITE && write_vio.nbytes != 0) { + SCOPED_MUTEX_LOCK(lock, write_vio.cont->mutex, this_ethread()); + write_vio.cont->handleEvent(event, &write_vio); + } else if (read_vio.cont && read_vio.op == VIO::READ) { + SCOPED_MUTEX_LOCK(lock, read_vio.cont->mutex, this_ethread()); + read_vio.cont->handleEvent(event, &read_vio); + } } break; From 45c076898fdbbd8f73b8017f8bb254cd0e6137c4 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 6 Jul 2021 08:56:59 +0900 Subject: [PATCH 3/4] Keep NetVC until every TXN is done on state_aborted --- proxy/http2/Http2ClientSession.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 46e98e581e2..252df2342b7 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -465,7 +465,8 @@ Http2ClientSession::state_goaway(int event, void *edata) /** Session State - Waiting all transaction is done (especially for background fill) - No actual read nor write operation. + The NetVC raised EOS or ERROR, but keep it until all transaction is done. ( Do NOT call vc->do_io_close() and set it nullptr ) + Because HttpSM assumes it's alive until everything is done. */ int Http2ClientSession::state_aborted(int event, void *edata) @@ -481,6 +482,16 @@ Http2ClientSession::state_aborted(int event, void *edata) break; } + case VC_EVENT_READ_READY: + case VC_EVENT_READ_COMPLETE: + case VC_EVENT_WRITE_READY: + case VC_EVENT_WRITE_COMPLETE: + case VC_EVENT_ERROR: + case VC_EVENT_EOS: { + // do nothing + Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); + break; + } default: Debug("unexpected event=%s (%d) edata=%p", get_vc_event_name(event), event, edata); ink_assert(false); @@ -1010,10 +1021,6 @@ Http2ClientSession::_handle_vc_event(int event, void *edata) dying_event = event; - // No more read/write op - _vc->do_io_close(); - _vc = nullptr; - // signal event to txns connection_state.cleanup_streams(event); From b516eca7b3f9397e97d23be7a270ec0c54f3898c Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Wed, 7 Jul 2021 15:35:52 +0900 Subject: [PATCH 4/4] Ignore GRACEFUL_SHUTDOWN event on state_aborted --- proxy/http2/Http2ClientSession.cc | 4 ++++ proxy/http2/Http2ClientSession.h | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 252df2342b7..e5dc25df635 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -482,6 +482,10 @@ Http2ClientSession::state_aborted(int event, void *edata) break; } + case HTTP2_SESSION_EVENT_GRACEFUL_SHUTDOWN: { + _graceful_shutdown_event = nullptr; + [[fallthrough]]; + } case VC_EVENT_READ_READY: case VC_EVENT_READ_COMPLETE: case VC_EVENT_WRITE_READY: diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h index 8c50ae280b0..becc15f8ee3 100644 --- a/proxy/http2/Http2ClientSession.h +++ b/proxy/http2/Http2ClientSession.h @@ -49,7 +49,9 @@ enum class Http2SsnMilestone { size_t const HTTP2_HEADER_BUFFER_SIZE_INDEX = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX; +// clang-format off /** + @startuml title ProxySession Continuation Handler hide empty description @@ -70,6 +72,8 @@ size_t const HTTP2_HEADER_BUFFER_SIZE_INDEX = CLIENT_CONNECTION_FIRST_READ_BUFFE state_aborted --> state_closed : all transaction is done state_open --> state_graceful_shutdown : graceful shutdown + state_graceful_shutdown --> state_aborted : VC_EVENT_EOS\lVC_EVENT_ERROR\lrecv GOAWAY frame with error code + state_graceful_shutdown --> state_goaway : VC_EVENT_ACTIVE_TIMEOUT\lVC_EVENT_INACTIVITY_TIMEOUT\lsend GOAWAY frame with error code state_graceful_shutdown --> state_closed : all transaction is done state_open --> state_goaway : VC_EVENT_ACTIVE_TIMEOUT\lVC_EVENT_INACTIVITY_TIMEOUT\lsend GOAWAY frame with error code @@ -91,6 +95,7 @@ size_t const HTTP2_HEADER_BUFFER_SIZE_INDEX = CLIENT_CONNECTION_FIRST_READ_BUFFE @enduml */ +// clang-format on class Http2ClientSession : public ProxySession { public: