TS-4916 Add safety net to avoid H2-infinite-loop deadlock.

proxy/http2/Http2ConnectionState.cc

@@ -908,6 +908,10 @@ Http2ConnectionState::create_stream(Http2StreamId new_id)
 
   Http2Stream *new_stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread());
   new_stream->init(new_id, client_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE));
+
+  ink_assert(NULL != new_stream);
+  ink_assert(!stream_list.in(new_stream));
+
   stream_list.push(new_stream);
   if (client_streamid) {
     latest_streamid_in = new_id;
@@ -938,6 +942,7 @@ Http2ConnectionState::find_stream(Http2StreamId id) const
     if (s->get_id() == id) {
       return s;
     }
+    ink_assert(s != s->link.next);
   }
   return NULL;
 }
@@ -952,6 +957,7 @@ Http2ConnectionState::restart_streams()
     if (!s->is_closed() && s->get_state() == HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE && min(this->client_rwnd, s->client_rwnd) > 0) {
       s->send_response_body();
     }
+    ink_assert(s != next);
     s = next;
   }
 }
@@ -963,22 +969,25 @@ Http2ConnectionState::cleanup_streams()
   while (s) {
     Http2Stream *next = s->link.next;
     this->delete_stream(s);
+    ink_assert(s != next);
     s = next;
   }
-  ink_assert(stream_list.head == NULL);
+  ink_assert(stream_list.empty());
 
   if (!is_state_closed()) {
     ua_session->get_netvc()->add_to_keep_alive_queue();
     ua_session->get_netvc()->cancel_active_timeout();
   }
 }
 
-void
+bool
 Http2ConnectionState::delete_stream(Http2Stream *stream)
 {
+  ink_assert(NULL != stream);
+
   // If stream has already been removed from the list, just go on
   if (!stream_list.in(stream)) {
-    return;
+    return false;
   }
 
   DebugHttp2Stream(ua_session, stream->get_id(), "Delete stream");
@@ -999,6 +1008,8 @@ Http2ConnectionState::delete_stream(Http2Stream *stream)
 
   stream_list.remove(stream);
   stream->initiating_close();
+
+  return true;
 }
 
 void

proxy/http2/Http2ConnectionState.h

@@ -174,7 +174,7 @@ class Http2ConnectionState : public Continuation
   Http2Stream *create_stream(Http2StreamId new_id);
   Http2Stream *find_stream(Http2StreamId id) const;
   void restart_streams();
-  void delete_stream(Http2Stream *stream);
+  bool delete_stream(Http2Stream *stream);
   void release_stream(Http2Stream *stream);
   void cleanup_streams();
 

proxy/http2/Http2Stream.cc

@@ -675,6 +675,18 @@ Http2Stream::destroy()
   }
   chunked_handler.clear();
   super::destroy();
+
+  // Current Http2ConnectionState implementation uses a memory pool for instantiating streams and DLL<> stream_list for storing
+  // active streams. Destroying a stream before deleting it from stream_list and then creating a new one + reusing the same chunk
+  // from the memory pool right away always leads to destroying the DLL structure (deadlocks, inconsistencies).
+  // The following is meant as a safety net since the consequences are disastrous. Until the design/implementation changes it seems
+  // less error prone to (double) delete before destroying (noop if already deleted).
+  if (parent) {
+    if (static_cast<Http2ClientSession *>(parent)->connection_state.delete_stream(this)) {
+      Warning("Http2Stream was about to be deallocated without removing it from the active stream list");
+    }
+  }
+
   THREAD_FREE(this, http2StreamAllocator, this_ethread());
 }