This is an automated email from the ASF dual-hosted git repository.
maskit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 4e5f024 Fix crash in H2 priority tree cleanup.
4e5f024 is described below
commit 4e5f024ebc0ea439371aa079f9a90a4bc419170e
Author: Susan Hinrichs <[email protected]>
AuthorDate: Tue Nov 7 00:08:36 2017 +0000
Fix crash in H2 priority tree cleanup.
---
proxy/http2/Http2ConnectionState.cc | 4 +++-
proxy/http2/Http2DependencyTree.h | 25 +++++++++++++++++++++++++
proxy/http2/Http2Stream.cc | 17 +++++------------
3 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/proxy/http2/Http2ConnectionState.cc
b/proxy/http2/Http2ConnectionState.cc
index 6b9d465..aa851a9 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -1125,6 +1125,7 @@ bool
Http2ConnectionState::delete_stream(Http2Stream *stream)
{
ink_assert(nullptr != stream);
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
// If stream has already been removed from the list, just go on
if (!stream_list.in(stream)) {
@@ -1140,7 +1141,9 @@ Http2ConnectionState::delete_stream(Http2Stream *stream)
dependency_tree->deactivate(node, 0);
}
dependency_tree->remove(node);
+ // ink_release_assert(dependency_tree->find(stream->get_id()) ==
nullptr);
}
+ stream->priority_node = nullptr;
}
if (stream->get_state() != Http2StreamState::HTTP2_STREAM_STATE_CLOSED) {
@@ -1166,7 +1169,6 @@ Http2ConnectionState::release_stream(Http2Stream *stream)
ink_assert(client_streams_out_count > 0);
--client_streams_out_count;
}
- stream_list.remove(stream);
}
if (ua_session) {
diff --git a/proxy/http2/Http2DependencyTree.h
b/proxy/http2/Http2DependencyTree.h
index d4b44ee..3f3af9b 100644
--- a/proxy/http2/Http2DependencyTree.h
+++ b/proxy/http2/Http2DependencyTree.h
@@ -119,6 +119,7 @@ public:
void activate(Node *node);
void deactivate(Node *node, uint32_t sent);
void update(Node *node, uint32_t sent);
+ bool in(Node *current, Node *node);
uint32_t size() const;
private:
@@ -202,6 +203,7 @@ Tree<T>::add(uint32_t parent_id, uint32_t id, uint32_t
weight, bool exclusive, T
parent->children.push(node);
if (!node->queue->empty()) {
+ ink_release_assert(!node->queued);
parent->queue->push(node->entry);
node->queued = true;
}
@@ -211,6 +213,27 @@ Tree<T>::add(uint32_t parent_id, uint32_t id, uint32_t
weight, bool exclusive, T
}
template <typename T>
+bool
+Tree<T>::in(Node *current, Node *node)
+{
+ bool retval = false;
+ if (current == nullptr)
+ current = _root;
+ if (current->queue->in(node->entry)) {
+ return true;
+ } else {
+ Node *child = current->children.head;
+ while (child) {
+ if (in(child, node)) {
+ return true;
+ }
+ child = child->link.next;
+ }
+ }
+ return retval;
+}
+
+template <typename T>
void
Tree<T>::remove(Node *node)
{
@@ -242,6 +265,8 @@ Tree<T>::remove(Node *node)
remove(parent);
}
+ // ink_release_assert(!this->in(nullptr, node));
+
--_node_count;
delete node;
}
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index 40a0834..30c1721 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -671,21 +671,14 @@ Http2Stream::destroy()
// Safe to initiate SSN_CLOSE if this is the last stream
if (parent) {
- // release_stream and delete_stream indirectly call each other and seem to
have a lot of commonality
- // Should get resolved at somepoint.
Http2ClientSession *h2_parent = static_cast<Http2ClientSession *>(parent);
SCOPED_MUTEX_LOCK(lock, h2_parent->connection_state.mutex, this_ethread());
- h2_parent->connection_state.release_stream(this);
+ // Make sure the stream is removed from the stream list and priority tree
+ // In many cases, this has been called earlier, so this call is a no-op
+ h2_parent->connection_state.delete_stream(this);
- // 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 (h2_parent->connection_state.delete_stream(this)) {
- Warning("Http2Stream was about to be deallocated without removing it
from the active stream list");
- }
+ // Update session's stream counts, so it accurately goes into keep-alive
state
+ h2_parent->connection_state.release_stream(this);
}
// Clean up the write VIO in case of inactivity timeout
--
To stop receiving notification emails like this one, please contact
['"[email protected]" <[email protected]>'].
