From: David Reiss <dreiss@apache.org>
Date: Sat, 2 Feb 2008 00:54:55 +0000 (+0000)
Subject: Thrift: Make borrow (almost) always succeed for TBufferedTransport.
X-Git-Tag: 0.2.0~1017
X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=f495f367c9d7dd89c15b9732a7ac8d0db43596bf;p=common%2Fthrift.git

Thrift: Make borrow (almost) always succeed for TBufferedTransport.

Chad Walters is writing a JSON protocol for Thrift, but he wants
borrow to always succeed.  That would be a pain to implement,
but here is a first step: borrow will almost always work with
TBufferedTransport.

Reviewed by: mcslee

Test Plan: Ran the DenseProtocol test and Zlib test, but more needs to be done.

Other Notes:
Also reviewed by Chad Walters, and maybe Ben Maurer.


git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665455 13f79535-47bb-0310-9956-ffa450edef68
---

diff --git a/lib/cpp/src/transport/TTransportUtils.cpp b/lib/cpp/src/transport/TTransportUtils.cpp
index b5e7fa85..556d5fa6 100644
--- a/lib/cpp/src/transport/TTransportUtils.cpp
+++ b/lib/cpp/src/transport/TTransportUtils.cpp
@@ -60,14 +60,42 @@ void TBufferedTransport::write(const uint8_t* buf, uint32_t len) {
 }
 
 const uint8_t* TBufferedTransport::borrow(uint8_t* buf, uint32_t* len) {
-  // Don't try to be clever with shifting buffers.
-  // If we have enough data, give a pointer to it,
-  // otherwise let the protcol use its slow path.
-  if (rLen_-rPos_ >= *len) {
+  // The number of additional bytes we need from the underlying transport.
+  // Could be zero or negative.
+  uint32_t need = *len - (rLen_-rPos_);
+
+  // If we have enough data, just hand over a pointer.
+  if (need <= 0) {
     *len = rLen_-rPos_;
     return rBuf_+rPos_;
   }
-  return NULL;
+
+  // If the request is bigger than our buffer, we are hosed.
+  if (*len > rBufSize_) {
+    return NULL;
+  }
+
+  // If we have less than half our buffer available,
+  // or we need more space than is in the buffer,
+  // shift the data we have down to the start.
+  if ((rLen_ > rBufSize_/2) || (rLen_+need > rBufSize_)) {
+    memmove(rBuf_, rBuf_+rPos_, rLen_-rPos_);
+    rLen_ -= rPos_;
+    rPos_ = 0;
+  }
+
+  // First try to fill up the buffer.
+  uint32_t got = transport_->read(rBuf_+rLen_, rBufSize_-rLen_);
+  rLen_ += got;
+  need -= got;
+
+  // If that fails, readAll until we get what we need.
+  if (need > 0) {
+    rLen_ += transport_->readAll(rBuf_+rLen_, need);
+  }
+
+  *len = rLen_-rPos_;
+  return rBuf_+rPos_;
 }
 
 void TBufferedTransport::consume(uint32_t len) {
diff --git a/lib/cpp/src/transport/TTransportUtils.h b/lib/cpp/src/transport/TTransportUtils.h
index ed64dec5..6aceb824 100644
--- a/lib/cpp/src/transport/TTransportUtils.h
+++ b/lib/cpp/src/transport/TTransportUtils.h
@@ -106,6 +106,17 @@ class TBufferedTransport : public TTransport {
 
   void flush();
 
+  /**
+   * The following behavior is currently implemented by TBufferedTransport,
+   * but that may change in a future version:
+   * 1/ If len is at most rBufSize_, borrow will never return NULL.
+   *    Depending on the underlying transport, it could throw an exception
+   *    or hang forever.
+   * 2/ Some borrow requests may copy bytes internally.  However,
+   *    if len is at most rBufSize_/2, none of the copied bytes
+   *    will ever have to be copied again.  For optimial performance,
+   *    stay under this limit.
+   */
   const uint8_t* borrow(uint8_t* buf, uint32_t* len);
 
   void consume(uint32_t len);