TS-4507: Fix SSN and TXN hook ordering.

proxy/ProxyClientSession.cc

@@ -67,7 +67,7 @@ is_valid_hook(TSHttpHookID hookid)
 }
 
 void
-ProxyClientSession::destroy()
+ProxyClientSession::free()
 {
   if (schedule_event) {
     schedule_event->cancel();
@@ -137,7 +137,7 @@ ProxyClientSession::state_api_callout(int event, void *data)
 
   // coverity[unterminated_default]
   default:
-    ink_assert(false);
+    ink_release_assert(false);
   }
 
   return 0;
@@ -185,7 +185,7 @@ ProxyClientSession::handle_api_return(int event)
       vc->do_io_close();
       this->release_netvc();
     }
-    this->destroy();
+    free(); // You can now clean things up
     break;
   }
   default:

proxy/ProxyClientSession.h

@@ -43,7 +43,8 @@ class ProxyClientSession : public VConnection
 public:
   ProxyClientSession();
 
-  virtual void destroy();
+  virtual void destroy() = 0;
+  virtual void free();
   virtual void start() = 0;
 
   virtual void new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader, bool backdoor) = 0;
@@ -188,6 +189,7 @@ class ProxyClientSession : public VConnection
   int64_t con_id;
 
   Event *schedule_event;
+  bool in_destroy;
 
 private:
   APIHookScope api_scope;

proxy/ProxyClientTransaction.cc

@@ -63,7 +63,7 @@ ProxyClientTransaction::release(IOBufferReader *r)
   DebugHttpTxn("[%" PRId64 "] session released by sm [%" PRId64 "]", parent ? parent->connection_id() : 0,
                current_reader ? current_reader->sm_id : 0);
 
-  current_reader = NULL; // Clear reference to SM
+  // current_reader = NULL; // Clear reference to SM
 
   // Pass along the release to the session
   if (parent) {
@@ -77,6 +77,16 @@ ProxyClientTransaction::attach_server_session(HttpServerSession *ssession, bool
   parent->attach_server_session(ssession, transaction_done);
 }
 
+void
+ProxyClientTransaction::destroy()
+{
+  if (current_reader) {
+    current_reader->ua_session = NULL;
+    current_reader             = NULL;
+  }
+  this->mutex.clear();
+}
+
 Action *
 ProxyClientTransaction::adjust_thread(Continuation *cont, int event, void *data)
 {

proxy/ProxyClientTransaction.h

@@ -174,11 +174,9 @@ class ProxyClientTransaction : public VConnection
     return true;
   }
 
-  virtual void
-  destroy()
-  {
-    this->mutex.clear();
-  }
+  virtual void destroy();
+
+  virtual void transaction_done() = 0;
 
   ProxyClientSession *
   get_parent()

proxy/http/Http1ClientSession.cc

@@ -31,15 +31,12 @@
  ****************************************************************************/
 
 #include <ts/ink_resolver.h>
-//#include "ink_config.h"
-//#include "Allocator.h"
 #include "Http1ClientSession.h"
 #include "Http1ClientTransaction.h"
 #include "HttpSM.h"
 #include "HttpDebugNames.h"
 #include "HttpServerSession.h"
 #include "Plugin.h"
-//#include "Http2ClientSession.h"
 
 #define DebugHttpSsn(fmt, ...) DebugSsn(this, "http_cs", fmt, __VA_ARGS__)
 
@@ -84,11 +81,23 @@ Http1ClientSession::Http1ClientSession()
 void
 Http1ClientSession::destroy()
 {
-  DebugHttpSsn("[%" PRId64 "] session destroy", con_id);
+  if (read_state != HCS_CLOSED) {
+    return;
+  }
+  if (!in_destroy) {
+    in_destroy = true;
+    DebugHttpSsn("[%" PRId64 "] session destroy", con_id);
 
-  ink_release_assert(!client_vc);
-  ink_assert(read_buffer);
+    ink_release_assert(!client_vc);
+    ink_assert(read_buffer);
 
+    do_api_callout(TS_HTTP_SSN_CLOSE_HOOK);
+  }
+}
+
+void
+Http1ClientSession::free()
+{
   magic = HTTP_CS_MAGIC_DEAD;
   if (read_buffer) {
     free_MIOBuffer(read_buffer);
@@ -112,7 +121,7 @@ Http1ClientSession::destroy()
   // Free the transaction resources
   this->trans.cleanup();
 
-  super::destroy();
+  super::free();
   THREAD_FREE(this, http1ClientSessionAllocator, this_thread());
 }
 
@@ -126,6 +135,7 @@ Http1ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
   mutex          = new_vc->mutex;
   trans.mutex    = mutex; // Share this mutex with the transaction
   ssn_start_time = Thread::get_hrtime();
+  in_destroy     = false;
 
   MUTEX_TRY_LOCK(lock, mutex, this_ethread());
   ink_assert(lock.is_locked());
@@ -215,6 +225,8 @@ Http1ClientSession::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader
 void
 Http1ClientSession::set_tcp_init_cwnd()
 {
+  if (!trans.get_sm())
+    return;
   int desired_tcp_init_cwnd = trans.get_sm()->t_state.txn_conf->server_tcp_init_cwnd;
   DebugHttpSsn("desired TCP congestion window is %d", desired_tcp_init_cwnd);
   if (desired_tcp_init_cwnd == 0) {
@@ -234,6 +246,8 @@ Http1ClientSession::do_io_shutdown(ShutdownHowTo_t howto)
 void
 Http1ClientSession::do_io_close(int alerrno)
 {
+  if (read_state == HCS_CLOSED)
+    return; // Don't double call session close
   if (read_state == HCS_ACTIVE_READER) {
     if (trans.m_active) {
       trans.m_active = false;
@@ -285,7 +299,13 @@ Http1ClientSession::do_io_close(int alerrno)
     HTTP_SUM_DYN_STAT(http_transactions_per_client_con, transact_count);
     HTTP_DECREMENT_DYN_STAT(http_current_client_connections_stat);
     conn_decrease = false;
-    do_api_callout(TS_HTTP_SSN_CLOSE_HOOK);
+    if (client_vc) {
+      client_vc->do_io_close();
+      client_vc = NULL;
+    }
+  }
+  if (trans.get_sm() == NULL) { // Destroying from keep_alive state
+    this->destroy();
   }
 }
 
@@ -314,6 +334,7 @@ Http1ClientSession::state_wait_for_close(int event, void *data)
     // Drain any data read
     sm_reader->consume(sm_reader->read_avail());
     break;
+
   default:
     ink_release_assert(0);
     break;

proxy/http/Http1ClientSession.h

@@ -56,6 +56,7 @@ class Http1ClientSession : public ProxyClientSession
 
   // Implement ProxyClientSession interface.
   virtual void destroy();
+  virtual void free();
 
   virtual void
   start()
@@ -92,7 +93,14 @@ class Http1ClientSession : public ProxyClientSession
   virtual void
   release_netvc()
   {
-    client_vc = NULL;
+    // Make sure the vio's are also released to avoid
+    // later surprises in inactivity timeout
+    if (client_vc) {
+      client_vc->do_io_read(NULL, 0, NULL);
+      client_vc->do_io_write(NULL, 0, NULL);
+      client_vc->set_action(NULL);
+      client_vc = NULL;
+    }
   }
 
   int
@@ -187,7 +195,12 @@ class Http1ClientSession : public ProxyClientSession
 
   MIOBuffer *read_buffer;
   IOBufferReader *sm_reader;
-  C_Read_State read_state;
+
+  /*
+   * Volatile should not be necessary, but there appears to be a bug in the 4.9 rhel gcc
+   * compiler that was using an old version of read_state to make decisions in really_destroy
+   */
+  volatile C_Read_State read_state;
 
   VIO *ka_vio;
   VIO *slave_ka_vio;

proxy/http/Http1ClientTransaction.cc

@@ -62,3 +62,22 @@ Http1ClientTransaction::set_parent(ProxyClientSession *new_parent)
   }
   super::set_parent(new_parent);
 }
+
+void
+Http1ClientTransaction::transaction_done()
+{
+  current_reader = NULL;
+  // If the parent session is not in the closed state, the destroy will not occur.
+  if (parent) {
+    parent->destroy();
+  }
+}
+
+void
+Http1ClientTransaction::destroy()
+{
+  if (current_reader) {
+    current_reader->ua_session = NULL;
+    current_reader             = NULL;
+  }
+}

proxy/http/Http1ClientTransaction.h

@@ -55,10 +55,7 @@ class Http1ClientTransaction : public ProxyClientTransaction
 
   // Don't destroy your elements.  Rely on the Http1ClientSession to clean up the
   // Http1ClientTransaction class as necessary
-  virtual void
-  destroy()
-  {
-  }
+  virtual void destroy();
 
   // Clean up the transaction elements when the ClientSession shuts down
   void
@@ -164,6 +161,7 @@ class Http1ClientTransaction : public ProxyClientTransaction
     if (parent)
       parent->cancel_inactivity_timeout();
   }
+  void transaction_done();
 
 protected:
   uint16_t outbound_port;

proxy/http/HttpSM.cc

@@ -910,11 +910,7 @@ HttpSM::state_watch_for_client_abort(int event, void *data)
       }
       ua_entry->eos = true;
     } else {
-      if (netvc) {
-        netvc->do_io_close();
-      }
       ua_session->do_io_close();
-      ua_session       = NULL;
       ua_buffer_reader = NULL;
       vc_table.cleanup_entry(ua_entry);
       ua_entry = NULL;
@@ -3007,9 +3003,8 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
       // Note: This is a hack. The correct solution is for the UA session to signal back to the SM
       // when the UA is about to be destroyed and clean up the pointer there. That should be done once
       // the TS-3612 changes are in place (and similarly for the server session).
-      if (ua_entry->in_tunnel) {
-        ua_session = NULL;
-      }
+      /*if (ua_entry->in_tunnel)
+        ua_session = NULL; */
 
       t_state.current.server->abort      = HttpTransact::ABORTED;
       t_state.client_info.keep_alive     = HTTP_NO_KEEPALIVE;
@@ -3325,12 +3320,11 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c)
     }
 
     ua_session->do_io_close();
-    ua_session = NULL;
   } else {
     ink_assert(ua_buffer_reader != NULL);
     ua_session->release(ua_buffer_reader);
     ua_buffer_reader = NULL;
-    ua_session       = NULL;
+    // ua_session       = NULL;
   }
 
   return 0;
@@ -6160,8 +6154,8 @@ HttpSM::setup_error_transfer()
   } else {
     DebugSM("http", "[setup_error_transfer] Now closing connection ...");
     vc_table.cleanup_entry(ua_entry);
-    ua_entry       = NULL;
-    ua_session     = NULL;
+    ua_entry = NULL;
+    // ua_session     = NULL;
     terminate_sm   = true;
     t_state.source = HttpTransact::SOURCE_INTERNAL;
   }
@@ -6775,7 +6769,6 @@ HttpSM::kill_this()
       plugin_tunnel = NULL;
     }
 
-    ua_session     = NULL;
     server_session = NULL;
 
     // So we don't try to nuke the state machine
@@ -6804,6 +6797,10 @@ HttpSM::kill_this()
   //   then the value of kill_this_async_done has changed so
   //   we must check it again
   if (kill_this_async_done == true) {
+    if (ua_session) {
+      ua_session->transaction_done();
+    }
+
     // In the async state, the plugin could have been
     // called resulting in the creation of a plugin_tunnel.
     // So it needs to be deleted now.