This is an automated email from the ASF dual-hosted git repository. zwoop pushed a commit to branch 9.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit a9870d9cf536ff952c3628dad722a356ec160b72 Author: Masakazu Kitajo <[email protected]> AuthorDate: Tue Mar 9 09:34:15 2021 +0900 Tidy up session/transaction destruction process (#7571) (cherry picked from commit fa603fd8c7878fe452218e45482fb4b5a0a726f5) --- proxy/ProxySession.cc | 25 +++--- proxy/ProxySession.h | 3 +- proxy/ProxyTransaction.cc | 13 ++-- proxy/ProxyTransaction.h | 2 +- proxy/http/Http1ClientSession.cc | 12 ++- proxy/http/Http1ClientSession.h | 1 + proxy/http/Http1ServerSession.cc | 7 ++ proxy/http/Http1ServerSession.h | 2 + proxy/http/Http1Transaction.cc | 2 +- proxy/http/Http1Transaction.h | 3 +- proxy/http2/Http2ClientSession.cc | 3 +- proxy/http2/Http2Stream.cc | 160 +++++++++++++++++++------------------- proxy/http2/Http2Stream.h | 2 +- proxy/http3/Http3Session.cc | 6 ++ proxy/http3/Http3Session.h | 1 + proxy/http3/Http3Transaction.cc | 6 -- proxy/http3/Http3Transaction.h | 1 - 17 files changed, 127 insertions(+), 122 deletions(-) diff --git a/proxy/ProxySession.cc b/proxy/ProxySession.cc index a866507..13d1399 100644 --- a/proxy/ProxySession.cc +++ b/proxy/ProxySession.cc @@ -30,6 +30,18 @@ ProxySession::ProxySession() : VConnection(nullptr) {} ProxySession::ProxySession(NetVConnection *vc) : VConnection(nullptr), _vc(vc) {} +ProxySession::~ProxySession() +{ + if (schedule_event) { + schedule_event->cancel(); + schedule_event = nullptr; + } + this->api_hooks.clear(); + this->mutex.clear(); + this->acl.clear(); + this->_ssl.reset(); +} + void ProxySession::set_session_active() { @@ -69,19 +81,6 @@ static const TSEvent eventmap[TS_HTTP_LAST_HOOK + 1] = { TS_EVENT_NONE, // TS_HTTP_LAST_HOOK }; -void -ProxySession::free() -{ - if (schedule_event) { - schedule_event->cancel(); - schedule_event = nullptr; - } - this->api_hooks.clear(); - this->mutex.clear(); - this->acl.clear(); - this->_ssl.reset(); -} - int ProxySession::state_api_callout(int event, void *data) { diff --git a/proxy/ProxySession.h b/proxy/ProxySession.h index d31888f..350236c 100644 --- a/proxy/ProxySession.h +++ b/proxy/ProxySession.h @@ -79,6 +79,7 @@ class ProxySession : public VConnection, public PluginUserArgs<TS_USER_ARGS_SSN> public: ProxySession(); ProxySession(NetVConnection *vc); + virtual ~ProxySession(); // noncopyable ProxySession(ProxySession &) = delete; @@ -94,7 +95,7 @@ public: virtual void release(ProxyTransaction *trans) = 0; virtual void destroy() = 0; - virtual void free(); + virtual void free() = 0; virtual void increment_current_active_connections_stat() = 0; virtual void decrement_current_active_connections_stat() = 0; diff --git a/proxy/ProxyTransaction.cc b/proxy/ProxyTransaction.cc index eedfa31..fff5fe7 100644 --- a/proxy/ProxyTransaction.cc +++ b/proxy/ProxyTransaction.cc @@ -28,6 +28,12 @@ ProxyTransaction::ProxyTransaction(ProxySession *session) : VConnection(nullptr), _proxy_ssn(session) {} +ProxyTransaction::~ProxyTransaction() +{ + this->_sm = nullptr; + this->mutex.clear(); +} + void ProxyTransaction::new_transaction(bool from_early_data) { @@ -62,13 +68,6 @@ ProxyTransaction::attach_server_session(PoolableSession *ssession, bool transact } void -ProxyTransaction::destroy() -{ - _sm = nullptr; - this->mutex.clear(); -} - -void ProxyTransaction::set_rx_error_code(ProxyError e) { if (this->_sm) { diff --git a/proxy/ProxyTransaction.h b/proxy/ProxyTransaction.h index 420a0a8..9767510 100644 --- a/proxy/ProxyTransaction.h +++ b/proxy/ProxyTransaction.h @@ -34,6 +34,7 @@ class ProxyTransaction : public VConnection public: ProxyTransaction() : VConnection(nullptr) {} ProxyTransaction(ProxySession *ssn); + virtual ~ProxyTransaction(); /// Virtual Methods // @@ -42,7 +43,6 @@ public: Action *adjust_thread(Continuation *cont, int event, void *data); virtual void release(IOBufferReader *r) = 0; virtual void transaction_done(); - virtual void destroy(); virtual void set_active_timeout(ink_hrtime timeout_in); virtual void set_inactivity_timeout(ink_hrtime timeout_in); diff --git a/proxy/http/Http1ClientSession.cc b/proxy/http/Http1ClientSession.cc index 8e269ca..3d24918 100644 --- a/proxy/http/Http1ClientSession.cc +++ b/proxy/http/Http1ClientSession.cc @@ -113,15 +113,12 @@ Http1ClientSession::free() conn_decrease = false; } - // Free the transaction resources - this->trans.super_type::destroy(); - if (_vc) { _vc->do_io_close(); _vc = nullptr; } - super::free(); + this->~Http1ClientSession(); THREAD_FREE(this, http1ClientSessionAllocator, this_thread()); } @@ -392,7 +389,8 @@ Http1ClientSession::release(ProxyTransaction *trans) // When release is called from start() to read the first transaction, get_sm() // will return null. - HttpSM *sm = trans->get_sm(); + HttpSM *sm = trans->get_sm(); + Http1Transaction *h1trans = static_cast<Http1Transaction *>(trans); if (sm) { MgmtInt ka_in = trans->get_sm()->t_state.txn_conf->keep_alive_no_activity_timeout_in; set_inactivity_timeout(HRTIME_SECONDS(ka_in)); @@ -410,7 +408,7 @@ Http1ClientSession::release(ProxyTransaction *trans) // IO to wait for new data bool more_to_read = this->_reader->is_read_avail_more_than(0); if (more_to_read) { - trans->destroy(); + h1trans->reset(); HttpSsnDebug("[%" PRId64 "] data already in buffer, starting new transaction", con_id); new_transaction(); } else { @@ -424,7 +422,7 @@ Http1ClientSession::release(ProxyTransaction *trans) _vc->cancel_active_timeout(); _vc->add_to_keep_alive_queue(); } - trans->destroy(); + h1trans->reset(); } } diff --git a/proxy/http/Http1ClientSession.h b/proxy/http/Http1ClientSession.h index 0c6e2f5..1509806 100644 --- a/proxy/http/Http1ClientSession.h +++ b/proxy/http/Http1ClientSession.h @@ -54,6 +54,7 @@ class Http1ClientSession : public ProxySession public: typedef ProxySession super; ///< Parent type. Http1ClientSession(); + ~Http1ClientSession() = default; // Implement ProxySession interface. void new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader) override; diff --git a/proxy/http/Http1ServerSession.cc b/proxy/http/Http1ServerSession.cc index b1b94fb..d800b03 100644 --- a/proxy/http/Http1ServerSession.cc +++ b/proxy/http/Http1ServerSession.cc @@ -50,6 +50,7 @@ Http1ServerSession::destroy() } mutex.clear(); + this->~Http1ServerSession(); if (httpSessionManager.get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_THREAD) { THREAD_FREE(this, httpServerSessionAllocator, this_thread()); } else { @@ -58,6 +59,12 @@ Http1ServerSession::destroy() } void +Http1ServerSession::free() +{ + // Unlike Http1ClientSession, Http1ServerSession is freed in destroy() +} + +void Http1ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader) { ink_assert(new_vc != nullptr); diff --git a/proxy/http/Http1ServerSession.h b/proxy/http/Http1ServerSession.h index 9594006..95e7ebc 100644 --- a/proxy/http/Http1ServerSession.h +++ b/proxy/http/Http1ServerSession.h @@ -56,11 +56,13 @@ public: Http1ServerSession() : super_type() {} Http1ServerSession(self_type const &) = delete; self_type &operator=(self_type const &) = delete; + ~Http1ServerSession() = default; //////////////////// // Methods void release(ProxyTransaction *) override; void destroy() override; + void free() override; // VConnection Methods void do_io_close(int lerrno = -1) override; diff --git a/proxy/http/Http1Transaction.cc b/proxy/http/Http1Transaction.cc index 6a37e7c..55e4d13 100644 --- a/proxy/http/Http1Transaction.cc +++ b/proxy/http/Http1Transaction.cc @@ -31,7 +31,7 @@ Http1Transaction::release(IOBufferReader *r) } void -Http1Transaction::destroy() // todo make ~Http1Transaction() +Http1Transaction::reset() { _sm = nullptr; } diff --git a/proxy/http/Http1Transaction.h b/proxy/http/Http1Transaction.h index 4769bd3..bd5b6b7 100644 --- a/proxy/http/Http1Transaction.h +++ b/proxy/http/Http1Transaction.h @@ -33,11 +33,11 @@ public: using super_type = ProxyTransaction; Http1Transaction(ProxySession *session) : super_type(session) {} + ~Http1Transaction() = default; //////////////////// // Methods void release(IOBufferReader *r) override; - void destroy() override; // todo make ~Http1Transaction() bool allow_half_open() const override; void transaction_done() override; @@ -45,6 +45,7 @@ public: void increment_client_transactions_stat() override; void decrement_client_transactions_stat() override; + void reset(); void set_reader(IOBufferReader *reader); //////////////////// diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 38a84d5..434b4a3 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -159,10 +159,9 @@ Http2ClientSession::free() delete _h2_pushed_urls; this->connection_state.destroy(); - super::free(); - free_MIOBuffer(this->read_buffer); free_MIOBuffer(this->write_buffer); + this->~Http2ClientSession(); THREAD_FREE(this, http2ClientSessionAllocator, this_ethread()); } diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index f1f362d..25a4d64 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -62,6 +62,81 @@ Http2Stream::Http2Stream(ProxySession *session, Http2StreamId sid, ssize_t initi http_parser_init(&http_parser); } +Http2Stream::~Http2Stream() +{ + REMEMBER(NO_EVENT, this->reentrancy_count); + Http2StreamDebug("Destroy stream, sent %" PRIu64 " bytes", this->bytes_sent); + SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); + // Clean up after yourself if this was an EOS + ink_release_assert(this->closed); + ink_release_assert(reentrancy_count == 0); + + uint64_t cid = 0; + + // Safe to initiate SSN_CLOSE if this is the last stream + if (_proxy_ssn) { + cid = _proxy_ssn->connection_id(); + + Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession *>(_proxy_ssn); + SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex, this_ethread()); + // 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`. + } + + // Clean up the write VIO in case of inactivity timeout + this->do_io_write(nullptr, 0, nullptr); + + this->_milestones.mark(Http2StreamMilestone::CLOSE); + + ink_hrtime total_time = this->_milestones.elapsed(Http2StreamMilestone::OPEN, Http2StreamMilestone::CLOSE); + HTTP2_SUM_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_TRANSACTIONS_TIME, this->_thread, total_time); + + // Slow Log + if (Http2::stream_slow_log_threshold != 0 && ink_hrtime_from_msec(Http2::stream_slow_log_threshold) < total_time) { + Error("[%" PRIu64 "] [%" PRIu32 "] [%" PRId64 "] Slow H2 Stream: " + "open: %" PRIu64 " " + "dec_hdrs: %.3f " + "txn: %.3f " + "enc_hdrs: %.3f " + "tx_hdrs: %.3f " + "tx_data: %.3f " + "close: %.3f", + cid, static_cast<uint32_t>(this->_id), this->_http_sm_id, + ink_hrtime_to_msec(this->_milestones[Http2StreamMilestone::OPEN]), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_DECODE_HEADERS), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TXN), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_ENCODE_HEADERS), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TX_HEADERS_FRAMES), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TX_DATA_FRAMES), + this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::CLOSE)); + } + + _req_header.destroy(); + response_header.destroy(); + + // Drop references to all buffer data + this->_request_buffer.clear(); + + // Free the mutexes in the VIO + read_vio.mutex.clear(); + write_vio.mutex.clear(); + + if (header_blocks) { + ats_free(header_blocks); + } + _clear_timers(); + clear_io_events(); + http_parser_clear(&http_parser); +} + int Http2Stream::main_event_handler(int event, void *edata) { @@ -407,7 +482,8 @@ Http2Stream::terminate_if_possible() Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession *>(this->_proxy_ssn); SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex, this_ethread()); - destroy(); + this->~Http2Stream(); + THREAD_FREE(this, http2StreamAllocator, this_ethread()); } } @@ -465,7 +541,8 @@ Http2Stream::initiating_close() } else if (!sent_write_complete) { // Transaction is already gone or not started. Kill yourself do_io_close(); - destroy(); + terminate_stream = true; + terminate_if_possible(); } } } @@ -744,85 +821,6 @@ Http2Stream::reenable(VIO *vio) } } -void -Http2Stream::destroy() -{ - REMEMBER(NO_EVENT, this->reentrancy_count); - Http2StreamDebug("Destroy stream, sent %" PRIu64 " bytes", this->bytes_sent); - SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - // Clean up after yourself if this was an EOS - ink_release_assert(this->closed); - ink_release_assert(reentrancy_count == 0); - - uint64_t cid = 0; - - // Safe to initiate SSN_CLOSE if this is the last stream - if (_proxy_ssn) { - cid = _proxy_ssn->connection_id(); - - Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession *>(_proxy_ssn); - SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex, this_ethread()); - // 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`. - } - - // Clean up the write VIO in case of inactivity timeout - this->do_io_write(nullptr, 0, nullptr); - - this->_milestones.mark(Http2StreamMilestone::CLOSE); - - ink_hrtime total_time = this->_milestones.elapsed(Http2StreamMilestone::OPEN, Http2StreamMilestone::CLOSE); - HTTP2_SUM_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_TRANSACTIONS_TIME, this->_thread, total_time); - - // Slow Log - if (Http2::stream_slow_log_threshold != 0 && ink_hrtime_from_msec(Http2::stream_slow_log_threshold) < total_time) { - Error("[%" PRIu64 "] [%" PRIu32 "] [%" PRId64 "] Slow H2 Stream: " - "open: %" PRIu64 " " - "dec_hdrs: %.3f " - "txn: %.3f " - "enc_hdrs: %.3f " - "tx_hdrs: %.3f " - "tx_data: %.3f " - "close: %.3f", - cid, static_cast<uint32_t>(this->_id), this->_http_sm_id, - ink_hrtime_to_msec(this->_milestones[Http2StreamMilestone::OPEN]), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_DECODE_HEADERS), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TXN), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_ENCODE_HEADERS), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TX_HEADERS_FRAMES), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::START_TX_DATA_FRAMES), - this->_milestones.difference_sec(Http2StreamMilestone::OPEN, Http2StreamMilestone::CLOSE)); - } - - _req_header.destroy(); - response_header.destroy(); - - // Drop references to all buffer data - this->_request_buffer.clear(); - - // Free the mutexes in the VIO - read_vio.mutex.clear(); - write_vio.mutex.clear(); - - if (header_blocks) { - ats_free(header_blocks); - } - _clear_timers(); - clear_io_events(); - http_parser_clear(&http_parser); - - super::destroy(); - THREAD_FREE(this, http2StreamAllocator, this_ethread()); -} - IOBufferReader * Http2Stream::response_get_data_reader() const { diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h index 44e6f0c..dfac308 100644 --- a/proxy/http2/Http2Stream.h +++ b/proxy/http2/Http2Stream.h @@ -56,10 +56,10 @@ public: Http2Stream() {} // Just to satisfy ClassAllocator Http2Stream(ProxySession *session, Http2StreamId sid, ssize_t initial_rwnd); + ~Http2Stream(); int main_event_handler(int event, void *edata); - void destroy() override; void release(IOBufferReader *r) override; void reenable(VIO *vio) override; void transaction_done() override; diff --git a/proxy/http3/Http3Session.cc b/proxy/http3/Http3Session.cc index 8e1168b..997e079 100644 --- a/proxy/http3/Http3Session.cc +++ b/proxy/http3/Http3Session.cc @@ -140,6 +140,12 @@ HQSession::destroy() } void +HQSession::free() +{ + delete this; +} + +void HQSession::release(ProxyTransaction *trans) { return; diff --git a/proxy/http3/Http3Session.h b/proxy/http3/Http3Session.h index 0d912ce..1663d1d 100644 --- a/proxy/http3/Http3Session.h +++ b/proxy/http3/Http3Session.h @@ -48,6 +48,7 @@ public: void new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader) override; void start() override; void destroy() override; + void free() override; void release(ProxyTransaction *trans) override; int get_transact_count() const override; diff --git a/proxy/http3/Http3Transaction.cc b/proxy/http3/Http3Transaction.cc index 5cd490b..c2f9de3 100644 --- a/proxy/http3/Http3Transaction.cc +++ b/proxy/http3/Http3Transaction.cc @@ -216,12 +216,6 @@ HQTransaction::reenable(VIO *vio) } void -HQTransaction::destroy() -{ - _sm = nullptr; -} - -void HQTransaction::transaction_done() { // TODO: start closing transaction diff --git a/proxy/http3/Http3Transaction.h b/proxy/http3/Http3Transaction.h index 033a052..00826ea 100644 --- a/proxy/http3/Http3Transaction.h +++ b/proxy/http3/Http3Transaction.h @@ -50,7 +50,6 @@ public: void cancel_inactivity_timeout() override; void transaction_done() override; bool allow_half_open() const override; - void destroy() override; void release(IOBufferReader *r) override; int get_transaction_id() const override; void increment_client_transactions_stat() override;
