diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 1fd0f8835af..c14ebf3f73a 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/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,9 +969,10 @@ 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(); @@ -973,12 +980,14 @@ Http2ConnectionState::cleanup_streams() } } -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 diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index eb6f93025ef..7f529272882 100644 --- a/proxy/http2/Http2ConnectionState.h +++ b/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(); diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index 8b80d1b9c15..4d3a07b22e5 100644 --- a/proxy/http2/Http2Stream.cc +++ b/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(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()); }