Properly handle appending to the tail of the chain

Summary: Currently appending to the tail of the chain would cause the cursor advancing to the beginning of the chain, which is not correct, instead we should advance to the tail.

Reviewed By: yfeldblum

Differential Revision: D6734999

fbshipit-source-id: b8b2585e0475b656f7b6bf4ed39686e2ccb2e432

diff --git a/folly/io/Cursor.h b/folly/io/Cursor.h
index 838d366bb52257222d89116bf468c472bde2a2ed..f74820dd48d10e382f37f8a6bcc3365fc43ab1ed 100644 (file)
--- a/folly/io/Cursor.h
+++ b/folly/io/Cursor.h
@@ -888,10 +888,19 @@ class RWCursor
       this->absolutePos_ += this->crtPos_ - this->crtBegin_;
       this->crtBuf_->appendChain(std::move(buf));
 
-      // Jump past the new links
-      this->crtBuf_ = nextBuf;
-      this->crtPos_ = this->crtBegin_ = this->crtBuf_->data();
-      this->crtEnd_ = this->crtBuf_->tail();
+      if (nextBuf == this->buffer_) {
+        // We've just appended to the end of the buffer, so advance to the end.
+        this->crtBuf_ = this->buffer_->prev();
+        this->crtBegin_ = this->crtBuf_->data();
+        this->crtPos_ = this->crtEnd_ = this->crtBuf_->tail();
+        // This has already been accounted for, so remove it.
+        this->absolutePos_ -= this->crtEnd_ - this->crtBegin_;
+      } else {
+        // Jump past the new links
+        this->crtBuf_ = nextBuf;
+        this->crtPos_ = this->crtBegin_ = this->crtBuf_->data();
+        this->crtEnd_ = this->crtBuf_->tail();
+      }
     }
   }
 

diff --git a/folly/io/test/IOBufCursorTest.cpp b/folly/io/test/IOBufCursorTest.cpp
index 1f013da621bf1db55d117d0e45a792280a9d3089..b9f6c50e8a3222cc2e5e8fdb5cd969cf0a894037 100644 (file)
--- a/folly/io/test/IOBufCursorTest.cpp
+++ b/folly/io/test/IOBufCursorTest.cpp
@@ -426,6 +426,22 @@ TEST(IOBuf, cloneAndInsert) {
     // Check that nextBuf got set correctly
     cursor.read<uint8_t>();
   }
+  {
+    // Check that inserting at the end of the buffer keeps it at the end.
+    RWPrivateCursor cursor(iobuf1.get());
+    Cursor(iobuf1.get()).clone(cloned, 1);
+    EXPECT_EQ(1, cloned->countChainElements());
+    EXPECT_EQ(1, cloned->computeChainDataLength());
+
+    cursor.advanceToEnd();
+    EXPECT_EQ(17, cursor.getCurrentPosition());
+    cursor.insert(std::move(cloned));
+    EXPECT_EQ(18, cursor.getCurrentPosition());
+    EXPECT_EQ(0, cursor.totalLength());
+    EXPECT_EQ(12, iobuf1->countChainElements());
+    EXPECT_EQ(18, iobuf1->computeChainDataLength());
+    EXPECT_TRUE(cursor.isAtEnd());
+  }
 }
 
 TEST(IOBuf, cloneWithEmptyBufAtStart) {