masaori335 commented on a change in pull request #7845:
URL: https://github.com/apache/trafficserver/pull/7845#discussion_r664152277
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -190,22 +191,34 @@ 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);
}
} else {
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);
}
Review comment:
Thanks for testing! It looks like I haven't tested cases in which
Http2Stream schedules events to itself.
> Don't unnderstand why we need to do this two times.
Right, we need only one time. We need a guard of null check of `e` for the
second block.
##########
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.
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -190,22 +191,34 @@ 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);
}
} else {
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);
}
Review comment:
f57bde4429f65dec7c0c3d2b03a6a190032b22e7 covers this (and probably the
last crash on `HttpSM::do_http_server_open`).
##########
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:
d53ac5c get rid of this and ignore VC events on state_aborted for just
in case.
--
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]
