You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by so...@apache.org on 2016/10/27 03:21:42 UTC
[trafficserver] branch 6.2.x updated: TS-4916 Add safety net to
avoid H2-infinite-loop deadlock.
This is an automated email from the ASF dual-hosted git repository.
sorber pushed a commit to branch 6.2.x
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/6.2.x by this push:
new eb1d019 TS-4916 Add safety net to avoid H2-infinite-loop deadlock.
eb1d019 is described below
commit eb1d0191da2f9fbc073da9fc03c6b004c9bca1d0
Author: Gancho Tenev <gt...@gmail.com>
AuthorDate: Mon Oct 17 15:10:12 2016 -0700
TS-4916 Add safety net to avoid H2-infinite-loop deadlock.
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).
Added 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).
(cherry picked from commit a6f9337f61c980739e08bbefeb5f6409b7dcdac1)
Conflicts:
proxy/http2/Http2ConnectionState.cc
---
proxy/http2/Http2ConnectionState.cc | 20 ++++++++++++++++----
proxy/http2/Http2ConnectionState.h | 2 +-
proxy/http2/Http2Stream.cc | 12 ++++++++++++
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index d1279c6..be863e1 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -892,6 +892,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);
latest_streamid = new_id;
@@ -909,8 +913,10 @@ Http2Stream *
Http2ConnectionState::find_stream(Http2StreamId id) const
{
for (Http2Stream *s = stream_list.head; s; s = s->link.next) {
- if (s->get_id() == id)
+ if (s->get_id() == id) {
return s;
+ }
+ ink_assert(s != s->link.next);
}
return NULL;
}
@@ -925,6 +931,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;
}
}
@@ -936,21 +943,24 @@ 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();
}
}
-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");
@@ -964,6 +974,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 5c50d81..0475b33 100644
--- a/proxy/http2/Http2ConnectionState.h
+++ b/proxy/http2/Http2ConnectionState.h
@@ -171,7 +171,7 @@ public:
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 cb42673..281a037 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -610,6 +610,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());
}
--
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].