masaori335 commented on a change in pull request #7845:
URL: https://github.com/apache/trafficserver/pull/7845#discussion_r664157367
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -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<UnixNetVConnection *>(_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<Event *>(edata);
+
+ // VIO will be reenableed in this handler
+ int res = (this->*session_handler)(VC_EVENT_READ_READY, static_cast<VIO
*>(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;
+
Review comment:
It looks like this is a race of callback from AIO and closing vc.
Actually, I don't understand why `HttpSM::state_cache_open_write` needs the
client vc. IMO, we can write to the cache without client vc.
> In the current code the _vc->do_io_close() is not called until everything
is shutdown and we are getting ready to free the Http2ClientSession object.
But this is what we have now. I'll adjust this PR to follow this assumption.
However, in the long term, we might want to change the design - e.g. when a
client is aborted & background-fill is working, we wouldn't want to keep
resources for the aborted client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
