fix review #3

Author: walterzhaoJR

src/butil/iobuf_inl.h

@@ -603,7 +603,7 @@ void remove_tls_block_chain();
 
 IOBuf::Block* acquire_tls_block();
 
-inline bool is_in_tls_block_chain(IOBuf::Block* head, IOBuf::Block* b) {
+static inline bool is_in_tls_block_chain(IOBuf::Block* head, IOBuf::Block* b) {
     for (IOBuf::Block* p = head; p != NULL; p = p->u.portal_next) {
         if (p == b) {
             return true;

test/iobuf_unittest.cpp

@@ -2001,19 +2001,27 @@ static void cleanup_corrupted_tls_chain(int drain_times) {
     dec_ref_distinct_blocks(b1, b2, b3);
 }
 
+struct CycleRegressionResult {
+    bool setup_ok;
+    bool has_cycle;
+};
+
 static void* double_return_release_tls_block_head_thread(void* arg) {
-    bool* has_cycle = static_cast<bool*>(arg);
+    CycleRegressionResult* result = static_cast<CycleRegressionResult*>(arg);
+    result->setup_ok = false;
+    result->has_cycle = false;
     butil::iobuf::remove_tls_block_chain();
 
     butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
     if (!b) {
         return NULL;
     }
+    result->setup_ok = true;
     butil::iobuf::release_tls_block(b);
     butil::iobuf::release_tls_block(b);
 
     if (tls_block_chain_has_cycle()) {
-        *has_cycle = true;
+        result->has_cycle = true;
         cleanup_corrupted_tls_chain(2);
     } else {
         butil::iobuf::remove_tls_block_chain();
@@ -2022,20 +2030,24 @@ static void* double_return_release_tls_block_head_thread(void* arg) {
 }
 
 TEST_F(IOBufTest, regression_3243_release_tls_block_head_no_cycle) {
-    bool has_cycle = false;
+    CycleRegressionResult result = {false, false};
     pthread_t tid;
     ASSERT_EQ(0, pthread_create(&tid, NULL,
                                 double_return_release_tls_block_head_thread,
-                                &has_cycle));
+                                &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
-    EXPECT_FALSE(has_cycle)
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to allocate a TLS block for regression setup.";
+    EXPECT_FALSE(result.has_cycle)
         << "release_tls_block() created a cycle when the same block was "
            "returned twice while already cached in TLS.  (GitHub issue #3243)";
 }
 
 static void* double_return_release_tls_block_non_head_thread(void* arg) {
-    bool* has_cycle = static_cast<bool*>(arg);
+    CycleRegressionResult* result = static_cast<CycleRegressionResult*>(arg);
+    result->setup_ok = false;
+    result->has_cycle = false;
     butil::iobuf::remove_tls_block_chain();
 
     butil::IOBuf::Block* tail = butil::iobuf::acquire_tls_block();
@@ -2044,6 +2056,7 @@ static void* double_return_release_tls_block_non_head_thread(void* arg) {
         dec_ref_distinct_blocks(tail, head);
         return NULL;
     }
+    result->setup_ok = true;
 
     butil::iobuf::release_tls_block(tail);
     butil::iobuf::release_tls_block(head);
@@ -2052,7 +2065,7 @@ static void* double_return_release_tls_block_non_head_thread(void* arg) {
     butil::iobuf::release_tls_block(tail);
 
     if (tls_block_chain_has_cycle()) {
-        *has_cycle = true;
+        result->has_cycle = true;
         cleanup_corrupted_tls_chain(3);
     } else {
         butil::iobuf::remove_tls_block_chain();
@@ -2061,33 +2074,38 @@ static void* double_return_release_tls_block_non_head_thread(void* arg) {
 }
 
 TEST_F(IOBufTest, regression_3243_release_tls_block_non_head_no_cycle) {
-    bool has_cycle = false;
+    CycleRegressionResult result = {false, false};
     pthread_t tid;
     ASSERT_EQ(0, pthread_create(&tid, NULL,
                                 double_return_release_tls_block_non_head_thread,
-                                &has_cycle));
+                                &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
-    EXPECT_FALSE(has_cycle)
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to allocate TLS blocks for regression setup.";
+    EXPECT_FALSE(result.has_cycle)
         << "release_tls_block() created a cycle when a block already present "
            "deeper in the TLS list was returned again.  "
            "(GitHub issue #3243)";
 }
 
 static void* double_return_release_tls_block_chain_head_thread(void* arg) {
-    bool* has_cycle = static_cast<bool*>(arg);
+    CycleRegressionResult* result = static_cast<CycleRegressionResult*>(arg);
+    result->setup_ok = false;
+    result->has_cycle = false;
     butil::iobuf::remove_tls_block_chain();
 
     butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
     if (!b) {
         return NULL;
     }
+    result->setup_ok = true;
     butil::iobuf::release_tls_block(b);
     b->u.portal_next = NULL;
     butil::iobuf::release_tls_block_chain(b);
 
     if (tls_block_chain_has_cycle()) {
-        *has_cycle = true;
+        result->has_cycle = true;
         cleanup_corrupted_tls_chain(2);
     } else {
         butil::iobuf::remove_tls_block_chain();
@@ -2096,20 +2114,24 @@ static void* double_return_release_tls_block_chain_head_thread(void* arg) {
 }
 
 TEST_F(IOBufTest, regression_3243_release_tls_block_chain_head_no_cycle) {
-    bool has_cycle = false;
+    CycleRegressionResult result = {false, false};
     pthread_t tid;
     ASSERT_EQ(0, pthread_create(&tid, NULL,
                                 double_return_release_tls_block_chain_head_thread,
-                                &has_cycle));
+                                &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
-    EXPECT_FALSE(has_cycle)
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to allocate a TLS block for chain regression setup.";
+    EXPECT_FALSE(result.has_cycle)
         << "release_tls_block_chain() created a cycle when the returned chain "
            "overlapped the TLS head.  (GitHub issue #3243)";
 }
 
 static void* double_return_release_tls_block_chain_non_head_thread(void* arg) {
-    bool* has_cycle = static_cast<bool*>(arg);
+    CycleRegressionResult* result = static_cast<CycleRegressionResult*>(arg);
+    result->setup_ok = false;
+    result->has_cycle = false;
     butil::iobuf::remove_tls_block_chain();
 
     butil::IOBuf::Block* tail = butil::iobuf::acquire_tls_block();
@@ -2118,6 +2140,7 @@ static void* double_return_release_tls_block_chain_non_head_thread(void* arg) {
         dec_ref_distinct_blocks(tail, head);
         return NULL;
     }
+    result->setup_ok = true;
 
     butil::iobuf::release_tls_block(tail);
     butil::iobuf::release_tls_block(head);
@@ -2127,7 +2150,7 @@ static void* double_return_release_tls_block_chain_non_head_thread(void* arg) {
     butil::iobuf::release_tls_block_chain(tail);
 
     if (tls_block_chain_has_cycle()) {
-        *has_cycle = true;
+        result->has_cycle = true;
         cleanup_corrupted_tls_chain(3);
     } else {
         butil::iobuf::remove_tls_block_chain();
@@ -2136,26 +2159,30 @@ static void* double_return_release_tls_block_chain_non_head_thread(void* arg) {
 }
 
 TEST_F(IOBufTest, regression_3243_release_tls_block_chain_non_head_no_cycle) {
-    bool has_cycle = false;
+    CycleRegressionResult result = {false, false};
     pthread_t tid;
     ASSERT_EQ(0, pthread_create(&tid, NULL,
                                 double_return_release_tls_block_chain_non_head_thread,
-                                &has_cycle));
+                                &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
-    EXPECT_FALSE(has_cycle)
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to allocate TLS blocks for chain regression setup.";
+    EXPECT_FALSE(result.has_cycle)
         << "release_tls_block_chain() created a cycle when the returned chain "
            "contained a block already present deeper in TLS.  "
            "(GitHub issue #3243)";
 }
 
 struct PartialOverlapResult {
+    bool setup_ok;
     bool has_cycle;
     int tls_block_count;
 };
 
 static void* partial_overlap_release_tls_block_chain_thread(void* arg) {
     PartialOverlapResult* result = static_cast<PartialOverlapResult*>(arg);
+    result->setup_ok = false;
     result->has_cycle = false;
     result->tls_block_count = -1;
     butil::iobuf::remove_tls_block_chain();
@@ -2167,6 +2194,7 @@ static void* partial_overlap_release_tls_block_chain_thread(void* arg) {
         dec_ref_distinct_blocks(tail, head, prefix);
         return NULL;
     }
+    result->setup_ok = true;
 
     butil::iobuf::release_tls_block(tail);
     butil::iobuf::release_tls_block(head);
@@ -2187,6 +2215,7 @@ static void* partial_overlap_release_tls_block_chain_thread(void* arg) {
 
 TEST_F(IOBufTest, regression_3243_release_tls_block_chain_partial_overlap_keeps_prefix) {
     PartialOverlapResult result;
+    result.setup_ok = false;
     result.has_cycle = false;
     result.tls_block_count = -1;
 
@@ -2196,6 +2225,8 @@ TEST_F(IOBufTest, regression_3243_release_tls_block_chain_partial_overlap_keeps_
                                 &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to allocate TLS blocks for partial-overlap regression setup.";
     EXPECT_FALSE(result.has_cycle)
         << "release_tls_block_chain() created a cycle when a unique prefix "
            "was returned ahead of a block already cached in TLS.  "
@@ -2209,40 +2240,49 @@ TEST_F(IOBufTest, regression_3243_release_tls_block_chain_partial_overlap_keeps_
 // eagerly returns _cur_block to TLS.  A subsequent release of the same pointer
 // used to create a self-loop at the head.
 static void* backup_double_return_thread(void* arg) {
-    bool* has_cycle = static_cast<bool*>(arg);
+    CycleRegressionResult* result = static_cast<CycleRegressionResult*>(arg);
+    result->setup_ok = false;
+    result->has_cycle = false;
     butil::iobuf::remove_tls_block_chain();
 
     butil::IOBuf buf;
     {
         butil::IOBufAsZeroCopyOutputStream stream(&buf);
         void* data = NULL;
         int size = 0;
-        EXPECT_TRUE(stream.Next(&data, &size));
+        if (!stream.Next(&data, &size)) {
+            return NULL;
+        }
         stream.BackUp(size);
 
         butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
-        EXPECT_TRUE(head != NULL);
+        if (head == NULL) {
+            return NULL;
+        }
+        result->setup_ok = true;
         butil::iobuf::release_tls_block(head);
 
         if (tls_block_chain_has_cycle()) {
-            *has_cycle = true;
+            result->has_cycle = true;
             cleanup_corrupted_tls_chain(2);
         }
     }
-    if (!*has_cycle) {
+    if (!result->has_cycle) {
         butil::iobuf::remove_tls_block_chain();
     }
     return NULL;
 }
 
 TEST_F(IOBufTest, regression_3243_backup_then_double_release_no_cycle) {
-    bool has_cycle = false;
+    CycleRegressionResult result = {false, false};
     pthread_t tid;
     ASSERT_EQ(0, pthread_create(&tid, NULL,
-                                backup_double_return_thread, &has_cycle));
+                                backup_double_return_thread, &result));
     ASSERT_EQ(0, pthread_join(tid, NULL));
 
-    EXPECT_FALSE(has_cycle)
+    ASSERT_TRUE(result.setup_ok)
+        << "Failed to set up IOBufAsZeroCopyOutputStream regression path.";
+    EXPECT_FALSE(result.has_cycle)
         << "After IOBufAsZeroCopyOutputStream::BackUp() returned a block to "
            "TLS, a second release_tls_block() created a cycle.  "
            "(GitHub issue #3243)";