From: Roger Meier <roger@apache.org>
Date: Wed, 11 Apr 2012 21:59:57 +0000 (+0000)
Subject: THRIFT-1562 Fix issue with TFramedTransport::readSlow
X-Git-Tag: 0.9.1~411
X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=02c827bcfe18c1ddb7450741a88bcb753eb1a063;p=common%2Fthrift.git

THRIFT-1562 Fix issue with TFramedTransport::readSlow
Patch: Dave Watson

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

diff --git a/lib/cpp/src/transport/TBufferTransports.cpp b/lib/cpp/src/transport/TBufferTransports.cpp
index 197a7ab4..b8a7ec3c 100644
--- a/lib/cpp/src/transport/TBufferTransports.cpp
+++ b/lib/cpp/src/transport/TBufferTransports.cpp
@@ -34,7 +34,7 @@ uint32_t TBufferedTransport::readSlow(uint8_t* buf, uint32_t len) {
   // with the data already in the buffer.
   assert(have < len);
 
-  // If we have some date in the buffer, copy it out and return it.
+  // If we have some data in the buffer, copy it out and return it.
   // We have to return it without attempting to read more, since we aren't
   // guaranteed that the underlying transport actually has more data, so
   // attempting to read from it could block.
@@ -140,11 +140,14 @@ uint32_t TFramedTransport::readSlow(uint8_t* buf, uint32_t len) {
   // with the data already in the buffer.
   assert(have < want);
 
-  // Copy out whatever we have.
+  // If we have some data in the buffer, copy it out and return it.
+  // We have to return it without attempting to read more, since we aren't
+  // guaranteed that the underlying transport actually has more data, so
+  // attempting to read from it could block.
   if (have > 0) {
     memcpy(buf, rBase_, have);
-    want -= have;
-    buf += have;
+    setReadBuffer(rBuf_.get(), 0);
+    return have;
   }
 
   // Read another frame.
diff --git a/lib/cpp/src/transport/TZlibTransport.h b/lib/cpp/src/transport/TZlibTransport.h
index 6bbff315..db765237 100644
--- a/lib/cpp/src/transport/TZlibTransport.h
+++ b/lib/cpp/src/transport/TZlibTransport.h
@@ -252,6 +252,24 @@ class TZlibTransport : public TVirtualTransport<TZlibTransport> {
   struct z_stream_s* wstream_;
 };
 
+
+/**
+ * Wraps a transport into a zlibbed one.
+ *
+ */
+class TZlibTransportFactory : public TTransportFactory {
+ public:
+  TZlibTransportFactory() {}
+
+  virtual ~TZlibTransportFactory() {}
+
+  virtual boost::shared_ptr<TTransport> getTransport(
+                                         boost::shared_ptr<TTransport> trans) {
+    return boost::shared_ptr<TTransport>(new TZlibTransport(trans));
+  }
+};
+
+
 }}} // apache::thrift::transport
 
 #endif // #ifndef _THRIFT_TRANSPORT_TZLIBTRANSPORT_H_
diff --git a/lib/cpp/test/TransportTest.cpp b/lib/cpp/test/TransportTest.cpp
index 767e5632..bc0529dc 100644
--- a/lib/cpp/test/TransportTest.cpp
+++ b/lib/cpp/test/TransportTest.cpp
@@ -561,6 +561,7 @@ void test_rw(uint32_t totalSize,
   BOOST_CHECK_EQUAL(memcmp(rbuf.get(), wbuf.get(), totalSize), 0);
 }
 
+
 template <class CoupledTransports>
 void test_read_part_available() {
   CoupledTransports transports;
@@ -583,6 +584,33 @@ void test_read_part_available() {
   clear_triggers();
 }
 
+template <class CoupledTransports>
+void test_read_part_available_in_chunks() {
+  CoupledTransports transports;
+  BOOST_REQUIRE(transports.in != NULL);
+  BOOST_REQUIRE(transports.out != NULL);
+
+  uint8_t write_buf[16];
+  uint8_t read_buf[16];
+  memset(write_buf, 'a', sizeof(write_buf));
+
+  // Write 10 bytes (in a single frame, for transports that use framing)
+  transports.out->write(write_buf, 10);
+  transports.out->flush();
+
+  // Read 1 byte, to force the transport to read the frame
+  uint32_t bytes_read = transports.in->read(read_buf, 1);
+  BOOST_CHECK_EQUAL(bytes_read, 1);
+
+  // Read more than what is remaining and verify the transport does not block
+  set_trigger(3, transports.out, 1);
+  bytes_read = transports.in->read(read_buf, 10);
+  BOOST_CHECK_EQUAL(numTriggersFired, 0);
+  BOOST_CHECK_EQUAL(bytes_read, 9);
+
+  clear_triggers();
+}
+
 template <class CoupledTransports>
 void test_read_partial_midframe() {
   CoupledTransports transports;
@@ -912,6 +940,12 @@ class TransportTestGen {
           test_read_part_available<CoupledTransports>, name);
     suite_->add(tc, expectedFailures);
 
+    snprintf(name, sizeof(name), "%s::test_read_part_available_in_chunks()",
+             transportName);
+    tc = boost::unit_test::make_test_case(
+          test_read_part_available_in_chunks<CoupledTransports>, name);
+    suite_->add(tc, expectedFailures);
+
     snprintf(name, sizeof(name), "%s::test_read_partial_midframe()",
              transportName);
     tc = boost::unit_test::make_test_case(