From 9a9f5cf3e2d4a53297ed13f6f365b557ea29f4c5 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Tue, 13 Oct 2020 16:52:33 +0000 Subject: [PATCH 1/4] Fix both the http and http2 current transaction count metrics --- proxy/ProxyTransaction.cc | 7 +------ proxy/http/Http1ClientSession.cc | 15 +++++++++++++-- proxy/http/Http1Transaction.cc | 17 +---------------- proxy/http/HttpSM.cc | 1 + proxy/http2/Http2Stream.cc | 5 +++-- 5 files changed, 19 insertions(+), 26 deletions(-) diff --git a/proxy/ProxyTransaction.cc b/proxy/ProxyTransaction.cc index 9be892ff999..6c48d210419 100644 --- a/proxy/ProxyTransaction.cc +++ b/proxy/ProxyTransaction.cc @@ -32,7 +32,7 @@ ProxyTransaction::ProxyTransaction() : VConnection(nullptr) {} void ProxyTransaction::new_transaction(bool from_early_data) { - ink_assert(_sm == nullptr); + ink_release_assert(_sm == nullptr); // Defensive programming, make sure nothing persists across // connection re-use @@ -63,11 +63,6 @@ ProxyTransaction::release(IOBufferReader *r) _sm ? _sm->sm_id : 0); this->decrement_client_transactions_stat(); - - // Pass along the release to the session - if (_proxy_ssn) { - _proxy_ssn->release(this); - } } void diff --git a/proxy/http/Http1ClientSession.cc b/proxy/http/Http1ClientSession.cc index 9177c0e4da3..7d68b248bf9 100644 --- a/proxy/http/Http1ClientSession.cc +++ b/proxy/http/Http1ClientSession.cc @@ -395,8 +395,19 @@ Http1ClientSession::release(ProxyTransaction *trans) { ink_assert(read_state == HCS_ACTIVE_READER || read_state == HCS_INIT); - // Timeout events should be delivered to the session - this->do_io_write(this, 0, nullptr); + // When release is called from start() to read the first transaction, get_sm() + // will return null. + HttpSM *sm = trans->get_sm(); + 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)); + + this->clear_session_active(); + this->ssn_last_txn_time = Thread::get_hrtime(); + + // Timeout events should be delivered to the session + this->do_io_write(this, 0, nullptr); + } // Check to see there is remaining data in the // buffer. If there is, spin up a new state diff --git a/proxy/http/Http1Transaction.cc b/proxy/http/Http1Transaction.cc index d2f8b2c3ad0..b6e807e52ff 100644 --- a/proxy/http/Http1Transaction.cc +++ b/proxy/http/Http1Transaction.cc @@ -28,22 +28,6 @@ void Http1Transaction::release(IOBufferReader *r) { - // Must set this inactivity count here rather than in the session because the state machine - // is not available then - MgmtInt ka_in = _sm->t_state.txn_conf->keep_alive_no_activity_timeout_in; - set_inactivity_timeout(HRTIME_SECONDS(ka_in)); - - _proxy_ssn->clear_session_active(); - _proxy_ssn->ssn_last_txn_time = Thread::get_hrtime(); - - // Make sure that the state machine is returning - // correct buffer reader - ink_assert(r == _reader); - if (r != _reader) { - this->do_io_close(); - } else { - super_type::release(r); - } } void @@ -55,6 +39,7 @@ Http1Transaction::destroy() // todo make ~Http1Transaction() void Http1Transaction::transaction_done() { + super_type::release(_reader); if (_proxy_ssn) { static_cast(_proxy_ssn)->release_transaction(); } diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 611e2732be7..1065970d680 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -3368,6 +3368,7 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c) } else { ink_assert(ua_buffer_reader != nullptr); ua_txn->release(ua_buffer_reader); + ua_txn->get_proxy_ssn()->release(ua_txn); ua_buffer_reader = nullptr; // ua_txn = NULL; } diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 417a6d6f325..e437224833a 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -49,6 +49,7 @@ Http2Stream::init(Http2StreamId sid, ssize_t initial_rwnd) { this->mark_milestone(Http2StreamMilestone::OPEN); + this->_sm = nullptr; this->_id = sid; this->_thread = this_ethread(); this->_client_rwnd = initial_rwnd; @@ -346,7 +347,6 @@ void Http2Stream::do_io_close(int /* flags */) { SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); - super::release(nullptr); if (!closed) { REMEMBER(NO_EVENT, this->reentrancy_count); @@ -754,6 +754,7 @@ Http2Stream::destroy() ink_release_assert(this->closed); ink_release_assert(reentrancy_count == 0); + super::release(nullptr); // Decrememt the current counts uint64_t cid = 0; // Safe to initiate SSN_CLOSE if this is the last stream @@ -894,10 +895,10 @@ Http2Stream::clear_io_events() } } +// release and do_io_close are the same for the HTTP/2 protocol void Http2Stream::release(IOBufferReader *r) { - super::release(r); this->do_io_close(); } From 0f492c3e58dca8579827df114e33007338d0cd6e Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 14 Oct 2020 13:22:41 +0000 Subject: [PATCH 2/4] Avoid remaining http/2 over decrement --- proxy/http2/Http2Stream.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index e437224833a..34a8dd42a7f 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -384,6 +384,10 @@ Http2Stream::transaction_done() cross_thread_event = nullptr; } + if (_sm) { + super::release(nullptr); // Decrememt the current counts + } + if (!closed) { do_io_close(); // Make sure we've been closed. If we didn't close the _proxy_ssn session better still be open } @@ -754,7 +758,6 @@ Http2Stream::destroy() ink_release_assert(this->closed); ink_release_assert(reentrancy_count == 0); - super::release(nullptr); // Decrememt the current counts uint64_t cid = 0; // Safe to initiate SSN_CLOSE if this is the last stream From dae4b831b43988e6e17d8191ba7f24683b100410 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 14 Oct 2020 13:43:51 +0000 Subject: [PATCH 3/4] Move decrement to transaction_done superclass --- proxy/ProxyTransaction.cc | 16 +++++++--------- proxy/ProxyTransaction.h | 4 ++-- proxy/http/Http1Transaction.cc | 3 ++- proxy/http2/Http2Stream.cc | 5 +---- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/proxy/ProxyTransaction.cc b/proxy/ProxyTransaction.cc index 6c48d210419..214c710c5ec 100644 --- a/proxy/ProxyTransaction.cc +++ b/proxy/ProxyTransaction.cc @@ -56,15 +56,6 @@ ProxyTransaction::new_transaction(bool from_early_data) _sm->attach_client_session(this, _reader); } -void -ProxyTransaction::release(IOBufferReader *r) -{ - HttpTxnDebug("[%" PRId64 "] session released by sm [%" PRId64 "]", _proxy_ssn ? _proxy_ssn->connection_id() : 0, - _sm ? _sm->sm_id : 0); - - this->decrement_client_transactions_stat(); -} - void ProxyTransaction::attach_server_session(Http1ServerSession *ssession, bool transaction_done) { @@ -192,3 +183,10 @@ ProxyTransaction::get_transaction_priority_dependence() const { return 0; } + +void +ProxyTransaction::transaction_done() +{ + SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); + this->decrement_client_transactions_stat(); +} diff --git a/proxy/ProxyTransaction.h b/proxy/ProxyTransaction.h index 83a2111610c..5ae71c84656 100644 --- a/proxy/ProxyTransaction.h +++ b/proxy/ProxyTransaction.h @@ -40,8 +40,8 @@ class ProxyTransaction : public VConnection virtual void new_transaction(bool from_early_data = false); virtual void attach_server_session(Http1ServerSession *ssession, bool transaction_done = true); Action *adjust_thread(Continuation *cont, int event, void *data); - virtual void release(IOBufferReader *r); - virtual void transaction_done() = 0; + virtual void release(IOBufferReader *r) = 0; + virtual void transaction_done(); virtual void destroy(); /// Virtual Accessors diff --git a/proxy/http/Http1Transaction.cc b/proxy/http/Http1Transaction.cc index b6e807e52ff..29ebec0da99 100644 --- a/proxy/http/Http1Transaction.cc +++ b/proxy/http/Http1Transaction.cc @@ -39,7 +39,8 @@ Http1Transaction::destroy() // todo make ~Http1Transaction() void Http1Transaction::transaction_done() { - super_type::release(_reader); + SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); + super_type::transaction_done(); if (_proxy_ssn) { static_cast(_proxy_ssn)->release_transaction(); } diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 34a8dd42a7f..acb955a849b 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -379,15 +379,12 @@ void Http2Stream::transaction_done() { SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); + super::transaction_done(); if (cross_thread_event) { cross_thread_event->cancel(); cross_thread_event = nullptr; } - if (_sm) { - super::release(nullptr); // Decrememt the current counts - } - if (!closed) { do_io_close(); // Make sure we've been closed. If we didn't close the _proxy_ssn session better still be open } From c3a3ad1dbcfa43adf3968cab59f698ce8f40e18e Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Wed, 14 Oct 2020 14:26:38 +0000 Subject: [PATCH 4/4] Update http3 transaction_done wiring --- proxy/http3/Http3Transaction.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/http3/Http3Transaction.cc b/proxy/http3/Http3Transaction.cc index 6ffb895e11b..ccb43a68cc6 100644 --- a/proxy/http3/Http3Transaction.cc +++ b/proxy/http3/Http3Transaction.cc @@ -108,7 +108,6 @@ HQTransaction::cancel_inactivity_timeout() void HQTransaction::release(IOBufferReader *r) { - super::release(r); this->do_io_close(); this->_sm = nullptr; } @@ -228,6 +227,7 @@ void HQTransaction::transaction_done() { // TODO: start closing transaction + super::transaction_done(); return; }