svn commit: r1325034 - in /thrift/trunk/lib/cpp: src/transport/TBufferTransports.cpp src/transport/TZlibTransport.h test/TransportTest.cpp

[ro...@apache.org]

Author: roger
Date: Wed Apr 11 21:59:57 2012
New Revision: 1325034

URL: http://svn.apache.org/viewvc?rev=1325034&view=rev
Log:
THRIFT-1562 Fix issue with TFramedTransport::readSlow
Patch: Dave Watson

Modified:
    thrift/trunk/lib/cpp/src/transport/TBufferTransports.cpp
    thrift/trunk/lib/cpp/src/transport/TZlibTransport.h
    thrift/trunk/lib/cpp/test/TransportTest.cpp

Modified: thrift/trunk/lib/cpp/src/transport/TBufferTransports.cpp
URL: http://svn.apache.org/viewvc/thrift/trunk/lib/cpp/src/transport/TBufferTransports.cpp?rev=1325034&r1=1325033&r2=1325034&view=diff
==============================================================================
--- thrift/trunk/lib/cpp/src/transport/TBufferTransports.cpp (original)
+++ thrift/trunk/lib/cpp/src/transport/TBufferTransports.cpp Wed Apr 11 21:59:57 2012
@@ -34,7 +34,7 @@ uint32_t TBufferedTransport::readSlow(ui
   // 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(uint
   // 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.

Modified: thrift/trunk/lib/cpp/src/transport/TZlibTransport.h
URL: http://svn.apache.org/viewvc/thrift/trunk/lib/cpp/src/transport/TZlibTransport.h?rev=1325034&r1=1325033&r2=1325034&view=diff
==============================================================================
--- thrift/trunk/lib/cpp/src/transport/TZlibTransport.h (original)
+++ thrift/trunk/lib/cpp/src/transport/TZlibTransport.h Wed Apr 11 21:59:57 2012
@@ -252,6 +252,24 @@ class TZlibTransport : public TVirtualTr
   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_

Modified: thrift/trunk/lib/cpp/test/TransportTest.cpp
URL: http://svn.apache.org/viewvc/thrift/trunk/lib/cpp/test/TransportTest.cpp?rev=1325034&r1=1325033&r2=1325034&view=diff
==============================================================================
--- thrift/trunk/lib/cpp/test/TransportTest.cpp (original)
+++ thrift/trunk/lib/cpp/test/TransportTest.cpp Wed Apr 11 21:59:57 2012
@@ -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;
@@ -584,6 +585,33 @@ void test_read_part_available() {
 }
 
 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;
   BOOST_REQUIRE(transports.in != NULL);
@@ -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(