[GitHub] [trafficserver] maskit commented on a change in pull request #7845: HTTP/2 Session & ConnectionState Cleanup

[GitBox <gi...@apache.org>]

maskit commented on a change in pull request #7845:
URL: https://github.com/apache/trafficserver/pull/7845#discussion_r639367763



##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -306,83 +332,264 @@ Http2ClientSession::main_event_handler(int event, void *edata)
   }
 
   switch (event) {
+  case VC_EVENT_READ_READY:
   case VC_EVENT_READ_COMPLETE:
-  case VC_EVENT_READ_READY: {
-    bool is_zombie = connection_state.get_zombie_event() != nullptr;
-    retval         = (this->*session_handler)(event, edata);
-    if (is_zombie && connection_state.get_zombie_event() != nullptr) {
-      Warning("Processed read event for zombie session %" PRId64, connection_id());
-    }
+  case VC_EVENT_WRITE_READY:
+  case VC_EVENT_WRITE_COMPLETE:
+  case VC_EVENT_ACTIVE_TIMEOUT:
+  case VC_EVENT_INACTIVITY_TIMEOUT:
+  case VC_EVENT_ERROR:
+  case VC_EVENT_EOS: {
+    _handle_vc_event(event, edata);
     break;
   }
-
-  case HTTP2_SESSION_EVENT_REENABLE:
+  case HTTP2_SESSION_EVENT_REENABLE: {
     // VIO will be reenableed in this handler
     retval = (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;
     break;
-
-  case VC_EVENT_ACTIVE_TIMEOUT:
-  case VC_EVENT_INACTIVITY_TIMEOUT:
-  case VC_EVENT_ERROR:
-  case VC_EVENT_EOS:
-    Http2SsnDebug("Closing event %d", event);
-    this->set_dying_event(event);
-    this->do_io_close();
+  }
+  default:
+    Http2SsnDebug("unexpected event=%d edata=%p", event, edata);
+    ink_release_assert(0);
     retval = 0;
     break;
+  }
+
+  recursion--;
+
+  if (recursion > 0) {
+    return retval;
+  }
+
+  //
+  // Bottom of the stack
+  //
+  if (_should_start_graceful_shutdown()) {
+    HTTP2_SET_CONT_HANDLER(&Http2ClientSession::state_graceful_shutdown);
+    connection_state.send_goaway_frame(INT32_MAX, Http2ErrorCode::HTTP2_ERROR_NO_ERROR);
+
+    // After allowing time for any in-flight stream creation (at least one round-trip time),
+    _graceful_shutdown_event = this_ethread()->schedule_in_local(this, HRTIME_SECONDS(2), HTTP2_SESSION_EVENT_GRACEFUL_SHUTDOWN);
+  }
+
+  if (handler == &Http2ClientSession::state_closed) {
+    clear_session_active();
+    destroy();
+  }
+
+  return retval;
+}
+
+int
+Http2ClientSession::_handle_vc_event(int event, void *edata)
+{
+  ink_release_assert(handler == &Http2ClientSession::state_open || handler == &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:
+  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();
     }
-    retval = 0;
+    res = 0;
     break;
+  }
+  case VC_EVENT_ACTIVE_TIMEOUT:
+  case VC_EVENT_INACTIVITY_TIMEOUT: {
+    HTTP2_SET_CONT_HANDLER(&Http2ClientSession::state_goaway);
+    Http2SsnDebug("state_goaway by %s (%d)", get_vc_event_name(event), event);
+
+    dying_event = event;
+
+    connection_state.send_goaway_frame(connection_state.get_latest_stream_id_in(), Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR);
 
-  case HTTP2_SESSION_EVENT_XMIT:
+    // Disable read - read is not needed by state_goaway
+    read_vio->done();
+
+    // Set nbyte to issue WRITE_COMPLETE event
+    int64_t len       = _write_buffer_reader->read_avail();
+    write_vio->nbytes = write_vio->ndone + len;
+
+    // signal timeout event to txns
+    connection_state.cleanup_streams(event);

Review comment:
       It may be too early to start removing dup code, but  I'd have this process in `_switch_to_goaway_state`. It'd also make debugging easy since we'd be able to set a breakpoint for the state change.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org