Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507762
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -0,0 +1,606 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <thrift/transport/THeaderTransport.h>
+#include <thrift/TApplicationException.h>
+#include <thrift/protocol/TProtocolTypes.h>
+
+#include <algorithm>
+#include <bitset>
+#include <cassert>
+#include <string>
+#include <zlib.h>
+
+using std::map;
+using boost::shared_ptr;
+using std::string;
+using std::vector;
+
+namespace apache { namespace thrift { namespace transport {
+
+using namespace apache::thrift::protocol;
+using apache::thrift::protocol::TBinaryProtocol;
+
+
+uint32_t THeaderTransport::readAll(uint8_t* buf, uint32_t len) {
+ // We want to call TBufferBase's version here, because
+ // TFramedTransport would try and call its own readFrame function
+ return TBufferBase::readAll(buf, len);
+}
+
+uint32_t THeaderTransport::readSlow(uint8_t* buf, uint32_t len) {
+
+ if (clientType == THRIFT_UNFRAMED_DEPRECATED) {
+ return transport_->read(buf, len);
+ }
+
+ return TFramedTransport::readSlow(buf, len);
+}
+
+uint16_t THeaderTransport::getProtocolId() const {
+ if (clientType == THRIFT_HEADER_CLIENT_TYPE) {
+ return protoId;
+ } else {
+ return T_BINARY_PROTOCOL; // Assume other transports use TBinary
+ }
+}
+
+void THeaderTransport::allocateReadBuffer(uint32_t sz) {
+ if (sz > rBufSize_) {
+ rBuf_.reset(new uint8_t[sz]);
+ rBufSize_ = sz;
+ }
+}
+
+bool THeaderTransport::readFrame(uint32_t minFrameSize) {
+ // szN is network byte order of sz
+ uint32_t szN;
+ uint32_t sz;
+
+ // Read the size of the next frame.
+ // We can't use readAll(&sz, sizeof(sz)), since that always throws an
+ // exception on EOF. We want to throw an exception only if EOF occurs
after
+ // partial size data.
+ uint32_t sizeBytesRead = 0;
+ while (sizeBytesRead < sizeof(szN)) {
+ uint8_t* szp = reinterpret_cast<uint8_t*>(&szN) + sizeBytesRead;
+ uint32_t bytesRead = transport_->read(szp, sizeof(szN) -
sizeBytesRead);
+ if (bytesRead == 0) {
+ if (sizeBytesRead == 0) {
+ // EOF before any data was read.
+ return false;
+ } else {
+ // EOF after a partial frame header. Raise an exception.
+ throw TTransportException(TTransportException::END_OF_FILE,
+ "No more data to read after "
+ "partial frame header.");
+ }
+ }
+ sizeBytesRead += bytesRead;
+ }
+
+ sz = ntohl(szN);
+
+ allocateReadBuffer(minFrameSize + 4);
+
+ if ((sz & TBinaryProtocol::VERSION_MASK) ==
(uint32_t)TBinaryProtocol::VERSION_1) {
--- End diff --
I am having a hard time figuring out which bytes on the wire mean what in
the context of THeaderTransport. I'll describe what I know of TBinaryProtocol
and TFramedTransport, and I'm hoping that you can fill in the blanks for
THeaderTransport.
For ancient, unframed binary protocol, the wire layout was something like
the following:
int32_t sizeOfStringBody, char stringBody[], int8_t messageType, int32_t
seqID
For the modern, unframed binary protocol, the wire layout takes advantage
of the int-ness of the old style, and does something like this:
int16_t version = 0x80010000, int8_t pad, int8_t messageType, int32_t
sizeOfStringBody, char stringBody[], int32_t seqID
Modern framed looks like this:
int32_t sizeOfFrame, int16_t version = 0x80010000, int8_t pad, int8_t
messageType, int32_t sizeOfStringBody, char stringBody[], int32_t seqID
Note that modern framed and ancient unframed are difficult to distinguish
from just looking at the size. Also note that at least one other version
number has been taken. See TBinaryProtocol.h
// VERSION_2 (0x80020000) is taken by TDenseProtocol.
I think what you are doing is the following... but I'm not sure:
int32_t sizeOfFrame, int16_t newVersion = 0x0FFF0000, int16_t flags,
int32_t seqID, int16_t sizeOfHeader, vint16_t protoID, vint16_t numTransforms,
and then more header stuff.
Couldn't we just use 0x80030000 instead of 0x0FFF0000? That seems like it
fits in with the current architecture better, and we won't have 'magic' numbers
running around as much. Alternatively, since this is a 'big' change... maybe
go with a version 0xC0000000.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
