diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 1bd78d417f7..c74ccf31b1b 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/proxy/http2/Http2ConnectionState.cc @@ -881,11 +881,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)); - stream_list.push(new_stream); + add_to_active_streams(new_stream); + latest_streamid = new_id; - ink_assert(client_streams_count < UINT32_MAX); - ++client_streams_count; new_stream->set_parent(ua_session); new_stream->mutex = ua_session->mutex; ua_session->get_netvc()->add_to_active_queue(); @@ -899,6 +898,7 @@ Http2ConnectionState::find_stream(Http2StreamId id) const for (Http2Stream *s = stream_list.head; s; s = s->link.next) { if (s->get_id() == id) return s; + ink_assert(s != s->link.next); } return NULL; } @@ -913,6 +913,7 @@ Http2ConnectionState::restart_streams() if (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; } } @@ -924,10 +925,13 @@ Http2ConnectionState::cleanup_streams() while (s) { Http2Stream *next = s->link.next; this->delete_stream(s); + ink_assert(s != next); s = next; } - client_streams_count = 0; + ink_assert(0 == client_streams_count); + ink_assert(NULL == stream_list.head); + if (!is_state_closed()) { ua_session->get_netvc()->add_to_keep_alive_queue(); } @@ -936,6 +940,13 @@ Http2ConnectionState::cleanup_streams() void Http2ConnectionState::delete_stream(Http2Stream *stream) { + // The following check allows the method to be called safely on already deleted streams. + if (deleted_from_active_streams(stream)) { + return; + } + + SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); + if (Http2::stream_priority_enabled) { DependencyTree::Node *node = stream->priority_node; if (node != NULL && node->active) { @@ -943,23 +954,56 @@ Http2ConnectionState::delete_stream(Http2Stream *stream) } } - stream_list.remove(stream); + rm_from_active_streams(stream); stream->initiating_close(); - ink_assert(client_streams_count > 0); - --client_streams_count; - if (client_streams_count == 0 && ua_session) { ua_session->get_netvc()->add_to_keep_alive_queue(); } } +inline bool +Http2ConnectionState::deleted_from_active_streams(Http2Stream *stream) +{ + // Is is a deleted DLL<> element ? + return !stream_list.in(stream); +} + +void +Http2ConnectionState::add_to_active_streams(Http2Stream *stream) +{ + // Make sure that we are not breaking the DLL<> structure. + ink_assert(NULL != stream); + ink_assert(!stream_list.in(stream)); + ink_assert(this->mutex->thread_holding == this_ethread()); + + stream_list.push(stream); + + ink_assert(client_streams_count < UINT32_MAX); + ++client_streams_count; +} + +void +Http2ConnectionState::rm_from_active_streams(Http2Stream *stream) +{ + // Make sure that we are not breaking the DLL<> structure. + ink_assert(NULL != stream); + ink_assert(stream_list.in(stream)); + ink_assert(this->mutex->thread_holding == this_ethread()); + + stream_list.remove(stream); + + ink_assert(client_streams_count > 0); + --client_streams_count; +} + void Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size) { // Update stream level window sizes for (Http2Stream *s = stream_list.head; s; s = s->link.next) { s->client_rwnd = new_size - (client_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE) - s->client_rwnd); + ink_assert(s != s->link.next); } } @@ -1097,6 +1141,7 @@ void Http2ConnectionState::send_data_frames(Http2Stream *stream) { if (stream->get_state() == HTTP2_STREAM_STATE_CLOSED) { + this->delete_stream(stream); return; } diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index 1594b9f7df0..d9d42632dc3 100644 --- a/proxy/http2/Http2ConnectionState.h +++ b/proxy/http2/Http2ConnectionState.h @@ -223,6 +223,12 @@ class Http2ConnectionState : public Continuation unsigned _adjust_concurrent_stream(); + // The following is to make sure stream_list and client_stream_count are updated together (in sync) + // with important asserts and checks to avoid breaking of DLL<> internal structure. + void add_to_active_streams(Http2Stream *stream); + void rm_from_active_streams(Http2Stream *stream); + bool deleted_from_active_streams(Http2Stream *stream); + // NOTE: 'stream_list' has only active streams. // If given Stream Identifier is not found in stream_list and it is less // than or equal to latest_streamid, the state of Stream diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc index f373a2d02a3..22c12d23abc 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -267,10 +267,12 @@ Http2Stream::do_io_close(int /* flags */) // Make sure any trailing end of stream frames are sent // Ourselve will be removed at send_data_frames or closing connection phase static_cast(parent)->connection_state.send_data_frames(this); + + // Make sure the stream is deleted at this point since next step is self destroy. + this->delete_stream(); } + parent = NULL; - // Check to see if the stream is in the closed state - ink_assert(get_state() == HTTP2_STREAM_STATE_CLOSED); clear_timers(); clear_io_events(); @@ -558,6 +560,14 @@ Http2Stream::reenable(VIO *vio) } } +void +Http2Stream::delete_stream() +{ + if (parent) { + static_cast(parent)->connection_state.delete_stream(this); + } +} + void Http2Stream::destroy() { @@ -591,6 +601,14 @@ 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). + this->delete_stream(); + THREAD_FREE(this, http2StreamAllocator, this_ethread()); } diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h index 17fc7093085..57fb3591ebe 100644 --- a/proxy/http2/Http2Stream.h +++ b/proxy/http2/Http2Stream.h @@ -233,6 +233,8 @@ class Http2Stream : public ProxyClientTransaction private: Event *send_tracked_event(Event *event, int send_event, VIO *vio); + void delete_stream(); + HTTPParser http_parser; ink_hrtime _start_time; EThread *_thread;