You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/07/13 20:44:26 UTC
[trafficserver] branch 8.1.x updated: Revert "Fix rare SSN/TXN
Start/Close Hook misorderings (#6364) (#6936)" (#6989)
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 8.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.1.x by this push:
new df36be7 Revert "Fix rare SSN/TXN Start/Close Hook misorderings (#6364) (#6936)" (#6989)
df36be7 is described below
commit df36be7f8a061249aa36ceac7f5cb0ac199727a7
Author: Masaori Koshiba <ma...@apache.org>
AuthorDate: Tue Jul 14 05:44:12 2020 +0900
Revert "Fix rare SSN/TXN Start/Close Hook misorderings (#6364) (#6936)" (#6989)
This reverts commit 0912e182a97a49b23af87c9ccdde159dbb5e86f8.
---
proxy/ProxyClientSession.cc | 7 +++----
proxy/ProxyClientSession.h | 2 +-
proxy/http/HttpSM.cc | 34 +++++++++-------------------------
proxy/http/HttpSM.h | 4 ++--
proxy/http2/Http2ClientSession.cc | 2 +-
proxy/http2/Http2ConnectionState.cc | 22 +++++++++++++++-------
proxy/http2/Http2ConnectionState.h | 8 +-------
proxy/http2/Http2Stream.cc | 6 +++---
8 files changed, 35 insertions(+), 50 deletions(-)
diff --git a/proxy/ProxyClientSession.cc b/proxy/ProxyClientSession.cc
index 44d8dca..1fe9daa 100644
--- a/proxy/ProxyClientSession.cc
+++ b/proxy/ProxyClientSession.cc
@@ -130,7 +130,7 @@ ProxyClientSession::state_api_callout(int event, void *data)
if (!schedule_event) { // Don't bother to schedule is there is already one out.
schedule_event = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10));
}
- return -1;
+ return 0;
}
}
@@ -160,7 +160,7 @@ ProxyClientSession::state_api_callout(int event, void *data)
return 0;
}
-int
+void
ProxyClientSession::do_api_callout(TSHttpHookID id)
{
ink_assert(id == TS_HTTP_SSN_START_HOOK || id == TS_HTTP_SSN_CLOSE_HOOK);
@@ -171,11 +171,10 @@ ProxyClientSession::do_api_callout(TSHttpHookID id)
if (this->hooks_on && this->has_hooks()) {
SET_HANDLER(&ProxyClientSession::state_api_callout);
- return this->state_api_callout(EVENT_NONE, nullptr);
+ this->state_api_callout(EVENT_NONE, nullptr);
} else {
this->handle_api_return(TS_EVENT_HTTP_CONTINUE);
}
- return 0;
}
void
diff --git a/proxy/ProxyClientSession.h b/proxy/ProxyClientSession.h
index 5e44262..76dc0c6 100644
--- a/proxy/ProxyClientSession.h
+++ b/proxy/ProxyClientSession.h
@@ -159,7 +159,7 @@ public:
}
// Initiate an API hook invocation.
- int do_api_callout(TSHttpHookID id);
+ void do_api_callout(TSHttpHookID id);
// Override if your session protocol allows this.
virtual bool
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 56e5e0e..13b4f7d 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -333,14 +333,13 @@ HttpSM::set_ua_half_close_flag()
ua_txn->set_half_close_flag(true);
}
-inline int
+inline void
HttpSM::do_api_callout()
{
if (hooks_set) {
- return do_api_callout_internal();
+ do_api_callout_internal();
} else {
handle_api_return();
- return 0;
}
}
@@ -366,16 +365,7 @@ HttpSM::state_add_to_list(int event, void * /* data ATS_UNUSED */)
}
t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_START;
- if (do_api_callout() < 0) {
- // Didn't get the hook continuation lock. Clear the read and wait for next event
- if (ua_entry->read_vio) {
- // Seems like ua_entry->read_vio->disable(); should work, but that was
- // not sufficient to stop the state machine from processing IO events until the
- // TXN_START hooks had completed
- ua_entry->read_vio = ua_entry->vc->do_io_read(nullptr, 0, nullptr);
- }
- return EVENT_CONT;
- }
+ do_api_callout();
return EVENT_DONE;
}
@@ -529,7 +519,6 @@ HttpSM::attach_client_session(ProxyClientTransaction *client_vc, IOBufferReader
++reentrancy_count;
// Add our state sm to the sm list
state_add_to_list(EVENT_NONE, nullptr);
-
// This is another external entry point and it is possible for the state machine to get terminated
// while down the call chain from @c state_add_to_list. So we need to use the reentrancy_count to
// prevent cleanup there and do it here as we return to the external caller.
@@ -1445,7 +1434,7 @@ plugins required to work with sni_routing.
// handler has been changed the value isn't important to the rest of the state machine
// but not resetting means there is no way to reliably detect re-entrance to this state with an
// outstanding callout.
- return -1;
+ return 0;
}
} else {
plugin_lock = false;
@@ -5049,12 +5038,12 @@ HttpSM::do_http_server_open(bool raw)
return;
}
-int
+void
HttpSM::do_api_callout_internal()
{
if (t_state.backdoor_request) {
handle_api_return();
- return 0;
+ return;
}
switch (t_state.api_next_action) {
@@ -5095,7 +5084,7 @@ HttpSM::do_api_callout_internal()
case HttpTransact::SM_ACTION_API_SM_SHUTDOWN:
if (callout_state == HTTP_API_IN_CALLOUT || callout_state == HTTP_API_DEFERED_SERVER_ERROR) {
callout_state = HTTP_API_DEFERED_CLOSE;
- return 0;
+ return;
} else {
cur_hook_id = TS_HTTP_TXN_CLOSE_HOOK;
}
@@ -5107,7 +5096,7 @@ HttpSM::do_api_callout_internal()
cur_hook = nullptr;
cur_hooks = 0;
- return state_api_callout(0, nullptr);
+ state_api_callout(0, nullptr);
}
VConnection *
@@ -6846,12 +6835,7 @@ HttpSM::kill_this()
// the terminate_flag
terminate_sm = false;
t_state.api_next_action = HttpTransact::SM_ACTION_API_SM_SHUTDOWN;
- if (do_api_callout() < 0) { // Failed to get a continuation lock
- // Need to hang out until we can complete the TXN_CLOSE hook
- terminate_sm = false;
- reentrancy_count--;
- return;
- }
+ do_api_callout();
}
// The reentrancy_count is still valid up to this point since
// the api shutdown hook is asynchronous and double frees can
diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h
index 5990d96..b9ce2d7 100644
--- a/proxy/http/HttpSM.h
+++ b/proxy/http/HttpSM.h
@@ -466,8 +466,8 @@ protected:
void do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_info, bool retry, bool allow_multiple = false);
void do_cache_delete_all_alts(Continuation *cont);
void do_auth_callout();
- int do_api_callout();
- int do_api_callout_internal();
+ void do_api_callout();
+ void do_api_callout_internal();
void do_redirect();
void redirect_request(const char *redirect_url, const int redirect_len);
void do_drain_request_body();
diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc
index f106250..6d7d3de 100644
--- a/proxy/http2/Http2ClientSession.cc
+++ b/proxy/http2/Http2ClientSession.cc
@@ -308,7 +308,7 @@ Http2ClientSession::do_io_close(int alerrno)
{
SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread());
- this->connection_state.release_stream();
+ this->connection_state.release_stream(nullptr);
}
this->clear_session_active();
diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index 27cdd20..394b299 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -531,7 +531,7 @@ rcv_rst_stream_frame(Http2ConnectionState &cstate, const Http2Frame &frame)
Http2StreamDebug(cstate.ua_session, stream_id, "RST_STREAM: Error Code: %u", rst_stream.error_code);
stream->set_rx_error_code({ProxyErrorClass::TXN, static_cast<uint32_t>(rst_stream.error_code)});
- stream->initiating_close();
+ cstate.delete_stream(stream);
}
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
@@ -1000,8 +1000,8 @@ Http2ConnectionState::main_event_handler(int event, void *edata)
ink_assert(this->fini_received == false);
this->fini_received = true;
cleanup_streams();
- release_stream();
SET_HANDLER(&Http2ConnectionState::state_closed);
+ this->release_stream(nullptr);
} break;
case HTTP2_SESSION_EVENT_XMIT: {
@@ -1298,11 +1298,13 @@ Http2ConnectionState::cleanup_streams()
if (this->tx_error_code.cls != ProxyErrorClass::NONE) {
s->set_tx_error_code(this->tx_error_code);
}
- s->initiating_close();
+ this->delete_stream(s);
ink_assert(s != next);
s = next;
}
+ ink_assert(stream_list.empty());
+
if (!is_state_closed()) {
SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());
@@ -1365,10 +1367,16 @@ Http2ConnectionState::delete_stream(Http2Stream *stream)
}
void
-Http2ConnectionState::release_stream()
+Http2ConnectionState::release_stream(Http2Stream *stream)
{
REMEMBER(NO_EVENT, this->recursion)
+ if (stream) {
+ // Decrement total_client_streams_count here, because it's a counter include streams in the process of shutting down.
+ // Other counters (client_streams_in_count/client_streams_out_count) are already decremented in delete_stream().
+ --total_client_streams_count;
+ }
+
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
if (this->ua_session) {
ink_assert(this->mutex == ua_session->mutex);
@@ -1467,7 +1475,7 @@ Http2ConnectionState::send_data_frames_depends_on_priority()
}
case Http2SendDataFrameResult::DONE: {
dependency_tree->deactivate(node, len);
- stream->initiating_close();
+ delete_stream(stream);
break;
}
default:
@@ -1565,7 +1573,7 @@ Http2ConnectionState::send_data_frames(Http2Stream *stream)
if (stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL ||
stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_CLOSED) {
Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown half closed local stream");
- stream->initiating_close();
+ this->delete_stream(stream);
return;
}
@@ -1580,7 +1588,7 @@ Http2ConnectionState::send_data_frames(Http2Stream *stream)
// RST_STREAM and WINDOW_UPDATE.
// See 'closed' state written at [RFC 7540] 5.1.
Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown stream");
- stream->initiating_close();
+ this->delete_stream(stream);
}
}
diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h
index 104d2f4..a30990e 100644
--- a/proxy/http2/Http2ConnectionState.h
+++ b/proxy/http2/Http2ConnectionState.h
@@ -176,7 +176,7 @@ public:
Http2Stream *find_stream(Http2StreamId id) const;
void restart_streams();
bool delete_stream(Http2Stream *stream);
- void release_stream();
+ void release_stream(Http2Stream *stream);
void cleanup_streams();
void restart_receiving(Http2Stream *stream);
void update_initial_rwnd(Http2WindowSize new_size);
@@ -228,12 +228,6 @@ public:
return client_streams_in_count;
}
- void
- decrement_stream_count()
- {
- --total_client_streams_count;
- }
-
double
get_stream_error_rate() const
{
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index d632a3a..6cbfe0a 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -410,6 +410,7 @@ Http2Stream::terminate_if_possible()
REMEMBER(NO_EVENT, this->reentrancy_count);
Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession *>(parent);
SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex, this_ethread());
+ h2_proxy_ssn->connection_state.delete_stream(this);
destroy();
}
}
@@ -779,10 +780,8 @@ Http2Stream::destroy()
// In many cases, this has been called earlier, so this call is a no-op
h2_proxy_ssn->connection_state.delete_stream(this);
- h2_proxy_ssn->connection_state.decrement_stream_count();
-
// Update session's stream counts, so it accurately goes into keep-alive state
- h2_proxy_ssn->connection_state.release_stream();
+ h2_proxy_ssn->connection_state.release_stream(this);
// Do not access `_proxy_ssn` in below. It might be freed by `release_stream`.
}
@@ -932,6 +931,7 @@ void
Http2Stream::release(IOBufferReader *r)
{
super::release(r);
+ current_reader = nullptr; // State machine is on its own way down.
this->do_io_close();
}