git commit: Adding unit test for HdrHeap coalesce crash (TS-2766)

[br...@apache.org]

Repository: trafficserver
Updated Branches:
  refs/heads/master 05f4db0d9 -> 1fec97e14


Adding unit test for HdrHeap coalesce crash (TS-2766)


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/1fec97e1
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/1fec97e1
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/1fec97e1

Branch: refs/heads/master
Commit: 1fec97e14491d9ecd2f90063ceeaea8dcea97a8e
Parents: 05f4db0
Author: Brian Geffon <br...@apache.org>
Authored: Mon May 19 15:33:37 2014 -0700
Committer: Brian Geffon <br...@apache.org>
Committed: Mon May 19 15:33:37 2014 -0700

----------------------------------------------------------------------
 proxy/hdrs/HdrHeap.cc | 124 +++++++++++++++++++++++++++------------------
 1 file changed, 76 insertions(+), 48 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1fec97e1/proxy/hdrs/HdrHeap.cc
----------------------------------------------------------------------
diff --git a/proxy/hdrs/HdrHeap.cc b/proxy/hdrs/HdrHeap.cc
index 296f766..cd762e6 100644
--- a/proxy/hdrs/HdrHeap.cc
+++ b/proxy/hdrs/HdrHeap.cc
@@ -1198,68 +1198,96 @@ struct StrTest
   int len;
 };
 
-#ifdef TESTS
-static void
-hdr_heap_test_verify(StrTest * str_test, int n)
-{
-  for (int i = 0; i < n; i++) {
-    char *ptr = str_test[i].ptr;
-    int len = str_test[i].len;
-
-    char c = (char) (len % 43);
-    for (int m = 0; m < len; m++) {
-      if (ptr[m] != c) {
-        Warning("Str Test failed on str %d of %d", i, n);
-        return;
-      }
-      c++;
-    }
+#if TS_HAS_TESTS
+#include <ts/TestBox.h>
+REGRESSION_TEST(HdrHeap_Coalesce)(RegressionTest* t, int /* atype ATS_UNUSED */, int*  pstatus) {
+  *pstatus = REGRESSION_TEST_PASSED;
+  /*
+   * This test is designed to test numerous pieces of the HdrHeaps including allocations,
+   * demotion of rw heaps to ronly heaps, and finally the coalesce and evacuate behaviours.
+   */
+
+  // The amount of space we will need to overflow the StrHdrHeap is HDR_STR_HEAP_DEFAULT_SIZE - STR_HEAP_HDR_SIZE
+  size_t next_rw_heap_size = HDR_STR_HEAP_DEFAULT_SIZE;
+  size_t next_required_overflow_size = next_rw_heap_size - STR_HEAP_HDR_SIZE;
+  char buf[next_required_overflow_size];
+  for(unsigned int i = 0; i < sizeof(buf); ++i) {
+    buf[i] = ('a' + (i % 26));
   }
 
-}
+  TestBox tb(t, pstatus);
+  HdrHeap *heap = new_HdrHeap();
+  URLImpl *url = url_create(heap);
 
-// NOTE: the tests are current broken since
-//   they don't have objects that can
-//   string evacuation
-void
-hdr_heap_test()
-{
-  Note("Starting hdr heap test");
+  tb.check(heap->m_read_write_heap.m_ptr == NULL, "Checking that we have no rw heap.");
+  url_path_set(heap,url, buf, next_required_overflow_size, true);
+  tb.check(heap->m_read_write_heap->m_free_size == 0, "Checking that we've completely consumed the rw heap");
+  for (int i = 0; i < HDR_BUF_RONLY_HEAPS; ++i) {
+    tb.check(heap->m_ronly_heap[i].m_heap_start == (char*)NULL, "Checking ronly_heap[%d] is NULL", i);
+  }
+
+  // Now we have no ronly heaps in use and a completely full rwheap, so we will test that
+  // we demote to ronly heaps HDR_BUF_RONLY_HEAPS times.
+  for (int ronly_heap = 0; ronly_heap < HDR_BUF_RONLY_HEAPS; ++ronly_heap) {
+    next_rw_heap_size = 2 * heap->m_read_write_heap->m_heap_size;
+    next_required_overflow_size = next_rw_heap_size - STR_HEAP_HDR_SIZE;
+    char buf2[next_required_overflow_size];
+    for(unsigned int i = 0; i < sizeof(buf2); ++i) {
+      buf2[i] = ('a' + (i % 26));
+    }
 
-  HdrHeap *h = new_HdrHeap();
+    URLImpl *url2 = url_create(heap);
+    url_path_set(heap,url2,buf2,next_required_overflow_size, true);
 
-  int s;
-  for (int i = 0; i < 10; i++) {
-    s = 16;
-    while (s < HDR_MAX_ALLOC_SIZE) {
-      h->allocate_obj(s);
-      s++;
+    tb.check(heap->m_read_write_heap->m_heap_size == (uint32_t)next_rw_heap_size, "Checking the current rw heap is %d bytes", next_rw_heap_size);
+    tb.check(heap->m_read_write_heap->m_free_size == 0, "Checking that we've completely consumed the rw heap");
+    tb.check(heap->m_ronly_heap[ronly_heap].m_heap_start != NULL, "Checking that we properly demoted the previous rw heap");
+    for (int i = ronly_heap + 1; i < HDR_BUF_RONLY_HEAPS; ++i) {
+      tb.check(heap->m_ronly_heap[i].m_heap_start == NULL, "Checking ronly_heap[%d] is NULL", i);
     }
   }
 
+  // We will rerun these checks after we introduce a non-copied string to make sure we didn't already coalesce
+  for (int i = 0; i < HDR_BUF_RONLY_HEAPS; ++i) {
+    tb.check(heap->m_ronly_heap[i].m_heap_start != (char*)NULL, "Pre non-copied string: Checking ronly_heap[%d] is NOT NULL", i);
+  }
 
-  // Allocate a bunch of strings
-  StrTest str_test[10000];
-  s = 16;
-  for (int j = 0; j < 10000; j++) {
-    str_test[j].ptr = h->allocate_str(s);
-    str_test[j].len = s;
+  // Now if we add a url object that contains only non-copied strings it shouldn't affect the size of the rwheap
+  // since it doesn't require allocating any storage on this heap.
+  char buf3[next_required_overflow_size];
+  for(unsigned int i = 0; i < sizeof(buf); ++i) {
+    buf3[i] = ('a' + (i % 26));
+  }
 
-    char c = (char) (s % 43);
-    for (int k = 0; k < s; k++) {
-      str_test[j].ptr[k] = c;
-      c++;
-    }
+  URLImpl *aliased_str_url = url_create(heap);
+  url_path_set(heap, aliased_str_url, buf3, next_required_overflow_size, false); // don't copy this string
+  tb.check(aliased_str_url->m_len_path == next_required_overflow_size, "Checking that the aliased string shows having proper length");
+  tb.check(aliased_str_url->m_ptr_path == buf3, "Checking that the aliased string is correctly pointing at buf");
 
-    hdr_heap_test_verify(str_test, j + 1);
-    s += 1;
+
+  for (int i = 0; i < HDR_BUF_RONLY_HEAPS; ++i) {
+    tb.check(heap->m_ronly_heap[i].m_heap_start != (char*)NULL, "Post non-copied string: Checking ronly_heap[%d] is NOT NULL", i);
   }
+  tb.check(heap->m_read_write_heap->m_free_size == 0, "Checking that we've completely consumed the rw heap");
+  tb.check(heap->m_next == NULL, "Checking that we dont have any chained heaps");
 
+  // Now at this point we have a completely full rw_heap and no ronly heap slots, so any allocation would have to result
+  // in a coalesce, and to validate that we don't reintroduce TS-2766 we have an aliased string, so when it tries to
+  // coalesce it used to sum up the size of the ronly heaps and the rw heap which is incorrect because we never
+  // copied the above string onto the heap. The new behaviour fixed in TS-2766 will make sure that this non copied
+  // string is accounted for, in the old implementation it would result in an allocation failure.
 
-  // Make sure the strings are all intact
-  hdr_heap_test_verify(str_test, 100);
 
-  Note("Str Test completed");
-}
+  char *str = heap->allocate_str(1); // this will force a coalese.
+  tb.check(str != NULL, "Checking that 1 byte allocated string is not NULL");
 
+  // Now we need to validate that aliased_str_url has a path that isn't NULL, if it's NULL then the
+  // coalesce is broken and didn't properly determine the size, if it's not null then everything worked as expected.
+  tb.check(aliased_str_url->m_len_path == next_required_overflow_size, "Checking that the aliased string shows having proper length");
+  tb.check(aliased_str_url->m_ptr_path != NULL, "Checking that the aliased string was properly moved during coalsece and evacuation");
+  tb.check(aliased_str_url->m_ptr_path != buf3, "Checking that the aliased string was properly moved during coalsece and evacuation (not pointing at buf3)");
+
+  // Clean up
+  heap->destroy();
+}
 #endif