You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by djwatson <gi...@git.apache.org> on 2015/01/16 22:22:23 UTC
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
GitHub user djwatson opened a pull request:
https://github.com/apache/thrift/pull/357
Facebook's THeader protocol and transport for cpp
Pulled from a pre-c++11 / folly / iobuf version. Removed some unused stuff.
This adds:
* headers
* dynamic protocol selection
* dynamic zlib compression
* backwards compat with unframed / framed
Added it to TestServer / TestClient. Tested various combinations, seems to work.
There were enough changes made vs. the fbthrift version that this could probably use a review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/djwatson/thrift THRIFT-2423
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/357.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #357
----
commit 8ff57a0b89b1cfaaa5d11d9fbf2cf70466f7ab7c
Author: Dave Watson <da...@fb.com>
Date: 2015-01-16T19:22:01Z
Facebook's THeader protocol and transport for cpp
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23508169
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
+ int16_t numTransforms;
+ ptr += readVarint16(ptr, &numTransforms, headerBoundary);
+
+ uint16_t macSz = 0;
+
+ // For now all transforms consist of only the ID, not data.
+ for (int i = 0; i < numTransforms; i++) {
+ int32_t transId;
+ ptr += readVarint32(ptr, &transId, headerBoundary);
+
+ readTrans_.push_back(transId);
+ }
+
+ // Info headers
+ while (ptr < headerBoundary) {
+ uint32_t infoId;
+ ptr += readVarint32(ptr, (int32_t*)&infoId, headerBoundary);
+
+ if (infoId == 0) {
+ // header padding
+ break;
+ }
+ if (infoId >= infoIdType::END) {
+ // cannot handle infoId
+ break;
+ }
+ switch (infoId) {
+ case infoIdType::KEYVALUE:
+ // Process key-value headers
+ uint32_t numKVHeaders;
+ ptr += readVarint32(ptr, (int32_t*)&numKVHeaders, headerBoundary);
+ // continue until we reach (padded) end of packet
+ while (numKVHeaders-- && ptr < headerBoundary) {
+ // format: key; value
+ // both: length (varint32); value (string)
+ string key, value;
+ readString(ptr, key, headerBoundary);
+ // value
+ readString(ptr, value, headerBoundary);
+ // save to headers
+ readHeaders_[key] = value;
+ }
+ break;
+ }
+ }
+
+ // Don't include the mac in the data to untransform
+ sz -= macSz;
+
+ // Untransform the data section. rBuf will contain result.
+ untransform(data, sz - (data - rBuf_.get())); // ignore header in size calc
+}
+
+void THeaderTransport::untransform(uint8_t* ptr, uint32_t sz) {
+ // Update the transform buffer size if needed
+ resizeTransformBuffer();
+
+ for (vector<uint16_t>::const_iterator it = readTrans_.begin();
+ it != readTrans_.end(); ++it) {
+ const uint16_t transId = *it;
+
+ if (transId == ZLIB_TRANSFORM) {
+ z_stream stream;
+ int err;
+
+ stream.next_in = ptr;
+ stream.avail_in = sz;
+
+ // Setting these to 0 means use the default free/alloc functions
+ stream.zalloc = (alloc_func)0;
+ stream.zfree = (free_func)0;
+ stream.opaque = (voidpf)0;
+ err = inflateInit(&stream);
+ if (err != Z_OK) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflateInit");
+ }
+ stream.next_out = tBuf_.get();
+ stream.avail_out = tBufSize_;
+ err = inflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END || stream.avail_out == 0) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflate");
+ }
+ sz = stream.total_out;
+
+ err = inflateEnd(&stream);
+ if (err != Z_OK) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflateEnd");
+ }
+
+ memcpy(ptr, tBuf_.get(), sz);
+ } else {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Unknown transform");
+ }
+ }
+
+ setReadBuffer(ptr, sz);
+}
+
+/**
+ * We may have updated the wBuf size, update the tBuf size to match.
+ * Should be called in transform.
+ *
+ * The buffer should be slightly larger than write buffer size due to
+ * compression transforms (that may slightly grow on small frame sizes)
+ */
+void THeaderTransport::resizeTransformBuffer() {
+ if (tBufSize_ < wBufSize_ + DEFAULT_BUFFER_SIZE) {
+ uint32_t new_size = wBufSize_ + DEFAULT_BUFFER_SIZE;
+ uint8_t* new_buf = new uint8_t[new_size];
+ tBuf_.reset(new_buf);
+ tBufSize_ = new_size;
+ }
+}
+
+void THeaderTransport::transform(uint8_t* ptr, uint32_t sz) {
+ // Update the transform buffer size if needed
+ resizeTransformBuffer();
+
+ for (vector<uint16_t>::const_iterator it = writeTrans_.begin();
+ it != writeTrans_.end(); ++it) {
+ const uint16_t transId = *it;
+
+ if (transId == ZLIB_TRANSFORM) {
+ z_stream stream;
+ int err;
+
+ stream.next_in = ptr;
+ stream.avail_in = sz;
+
+ stream.zalloc = (alloc_func)0;
+ stream.zfree = (free_func)0;
+ stream.opaque = (voidpf)0;
+ err = deflateInit(&stream, Z_DEFAULT_COMPRESSION);
+ if (err != Z_OK) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Error while zlib deflateInit");
+ }
+ stream.next_out = tBuf_.get();
+ stream.avail_out = tBufSize_;
+ err = deflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END || stream.avail_out == 0) {
--- End diff --
This implementation seems to assume that deflating something will add 512 bytes at most to the message size. I think a loop and some reallocation of tBuf_ would be appropriate here.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23559536
--- 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'll add this doc to the commit: https://github.com/facebook/fbthrift/blob/master/thrift/doc/HeaderFormat.md
Yea I can't think of any reason the magic couldn't be 8003 vs 0FFF
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-77480261
fixed zlib check to be == Z_OK. Rebased to apache/master
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507741
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
--- End diff --
I'm not thrilled with VarInts here. I suspect that for the Facebook use case, you are stuck with them, because you are probably already running lots of services with VarInts in the headers.
Why am I not fond of VarInts? They save space on the wire, so that must be good, right? Well, for most use cases, the slow part of networking is because of latency, not throughput. Stated another way, round trips are expensive, but extra bytes in the same round trip tend not to be. On a gigabit network, a byte costs roughly 8 nanoseconds, and VarInt16 will only save one byte in its best case. So let's translate this to processor time now. Assume a 3 GHz processor. 3 cycles fit into one nanosecond. Does the entirety of the code required to deal with VarInts in the good case fit in 24 cycles? If it takes more cycles, then you have slowed things down. Now, this is a gross simplification in many ways, as it doesn't account for caches, or the work that the network stacks have to perform to send one more byte... but it does mean that VarInts aren't a clear win.
Now maybe this doesn't apply to Facebook. I don't know whether they are more bandwidth bound or CPU bound, and I don't know whether CPU time or network time consumes more power. Either way though, I am skeptical of this design choice. Maybe we can get the best for Facebook and the best for Apache by using a different version / magic number to distinguish the cases. Or maybe we call this TCompactHeader and something else TBinaryHeader.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23559234
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
--- End diff --
In Facebook's testing, varints in CompactProtocol were always faster (by at least 20%) than BinaryProtocol (we didn't test theader directly). It did involve some gross unrolling of loops though:
https://github.com/facebook/fbthrift/blob/master/thrift/lib/cpp/util/VarintUtils.tcc
Protocolbufs does similar unrolling.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23307400
--- Diff: lib/cpp/src/thrift/protocol/TProtocol.h ---
@@ -659,6 +659,11 @@ class TProtocolFactory {
virtual ~TProtocolFactory() {}
virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> trans) = 0;
+ virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> inTrans,
--- End diff --
THeader detects the client transport type - unframed, framed, header - and responds in kind. To make this work, the input/output transport need to be the *same* THeader instance.
The transports don't necessarily need to be the same though (and in the case of TNonblocking, need to be different due to the way the MemoryBuffers work). So THeader here takes in two transports, but uses a single protocol.
Actually in the fbthrift code this wasn't added to the interface, but just special cased in TNonblockingServer with a dynamic_cast to TheaderProtocol, which we could do instead
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23505515
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
--- End diff --
This seems wrong. The preceding comment says "Don't include the unknown client", which is fine, but the last known client type has a value of '2'. That suggests that this define should be for '3'. I suspect that there used to be a '3' in some super-secret facebook stuff :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23162158
--- Diff: lib/cpp/src/thrift/protocol/TProtocolTypes.h ---
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_PROTOCOL_TPROTOCOLTYPES_H_
+#define THRIFT_PROTOCOL_TPROTOCOLTYPES_H_ 1
+
+namespace apache { namespace thrift { namespace protocol {
+
+enum PROTOCOL_TYPES {
+ T_BINARY_PROTOCOL = 0,
+ T_JSON_PROTOCOL = 1,
+ T_COMPACT_PROTOCOL = 2,
+};
--- End diff --
One day (probably not today), we should figure out how to share constants like these across all the languages.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507888
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
--- End diff --
pointer overflow risk. If strLen is huge on a 32-bit system, then ptr will wrap around and skip this check.
This construct will avoid any overflow possibilities, because you know that headerBoundary and ptr start out relatively close to one another:
if(strLen > headerBoundary - ptr) {throw ...}
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23161617
--- Diff: lib/cpp/src/thrift/protocol/THeaderProtocol.cpp ---
@@ -0,0 +1,260 @@
+/*
+ * 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.
+ */
+#ifndef THRIFT_PROTOCOL_THEADERPROTOCOL_CPP_
+#define THRIFT_PROTOCOL_THEADERPROTOCOL_CPP_ 1
+
+#include <thrift/protocol/THeaderProtocol.h>
+#include <thrift/protocol/TCompactProtocol.h>
+#include <thrift/TApplicationException.h>
+
+#include <limits>
+#include <boost/static_assert.hpp>
+
+
+namespace apache { namespace thrift { namespace protocol {
+
+void THeaderProtocol::resetProtocol() {
+ if (proto_ && protoId_ == trans_->getProtocolId()) {
+ return;
+ }
+
+ protoId_ = trans_->getProtocolId();
+
+ switch (protoId_) {
+ case T_BINARY_PROTOCOL:
+ proto_ = boost::shared_ptr<TProtocol>(
--- End diff --
One of the many things I've been meaning to do is to make a big sweep over the cpp code and change boost::shared_ptr creations to use boost::make_shared.
If you have time, you can make that change in your new files. If not, that is fine also. It can wait until someone goes through and changes them across the board.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507551
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
--- End diff --
potential integer overflow here
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23304519
--- Diff: lib/cpp/src/thrift/protocol/TProtocol.h ---
@@ -659,6 +659,11 @@ class TProtocolFactory {
virtual ~TProtocolFactory() {}
virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> trans) = 0;
+ virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> inTrans,
--- End diff --
I'm not sure why this getProtocol overload is here. All the servers have an inputProtocolFactory and outputProtocolFactory. Maybe this is a little bit more usable... but it adds unusual noise to the review. I would like to have some justification for this overload in regards to THeader... or I would like to see this pulled into its own issue.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23305015
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
--- End diff --
This needs to be thrift/cxxfunctional.h, and usages of ::std::tr1::function need to be replaced with tcxx::function. Clang doesn't have the tr1 headers... but they do have the C++11 headers, and thrift/cxxfunctional.h deals with that.
Or maybe it just needs to be removed? I don't see a tr1::function lying around anywhere.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-75660559
Just FYI additional commit added to PR covering review comments
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-76593090
could you rebase to master and I commit
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2423: Facebook's THeader protocol and ...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-153325696
@nsuke Yes, please do so.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r25197742
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
+
+enum CLIENT_TYPE {
+ THRIFT_HEADER_CLIENT_TYPE = 0,
+ THRIFT_FRAMED_DEPRECATED = 1,
+ THRIFT_UNFRAMED_DEPRECATED = 2,
+ THRIFT_UNKNOWN_CLIENT_TYPE = 4,
+};
+
+namespace apache { namespace thrift { namespace transport {
+
+using apache::thrift::protocol::T_COMPACT_PROTOCOL;
+
+/**
+ * Header transport. All writes go into an in-memory buffer until flush is
+ * called, at which point the transport writes the length of the entire
+ * binary chunk followed by the data payload. This allows the receiver on the
+ * other end to always do fixed-length reads.
+ *
+ * Subclass TFramedTransport because most of the read/write methods are similar
--- End diff --
Within the Thrift code base, nothing passes around pointers to TBufferedTransport objects. Some of the async classes pass around pointers to TBufferBase objects though. If you are trying to make THeaderTransport fit in the async classes, then inheriting from TBufferBase should be enough.
If you don't want / don't care about the async classes, then I don't see the benefit in inheriting from the TBuffer classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507819
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -547,12 +553,18 @@ void TNonblockingServer::TConnection::transition() {
case APP_READ_REQUEST:
// We are done reading the request, package the read buffer into transport
// and get back some data from the dispatch function
- inputTransport_->resetBuffer(readBuffer_, readBufferPos_);
- outputTransport_->resetBuffer();
- // Prepend four bytes of blank space to the buffer so we can
- // write the frame size there later.
- outputTransport_->getWritePtr(4);
- outputTransport_->wroteBytes(4);
+ if (!server_->getOutputProtocolFactory()) {
--- End diff --
If I understand this correctly, then this is basically asking "are we in the THeader case". I think that this information should be stored in a well named member enum, and then we can query that. The current query doesn't really express intent to me.
Alternatively, comment the reason for the difference.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-76320937
Looks pretty good. Just the one last zlib comment and I think everything will be good to go.
Thanks for putting up with my nitpicky comments :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23308416
--- Diff: lib/cpp/src/thrift/protocol/TProtocol.h ---
@@ -659,6 +659,11 @@ class TProtocolFactory {
virtual ~TProtocolFactory() {}
virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> trans) = 0;
+ virtual boost::shared_ptr<TProtocol> getProtocol(boost::shared_ptr<TTransport> inTrans,
--- End diff --
This makes sense. I would like that information captured in a comment somewhere. Maybe in THeaderTransport.h?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23161968
--- Diff: lib/cpp/src/thrift/protocol/THeaderProtocol.cpp ---
@@ -0,0 +1,260 @@
+/*
+ * 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.
+ */
+#ifndef THRIFT_PROTOCOL_THEADERPROTOCOL_CPP_
+#define THRIFT_PROTOCOL_THEADERPROTOCOL_CPP_ 1
+
+#include <thrift/protocol/THeaderProtocol.h>
+#include <thrift/protocol/TCompactProtocol.h>
+#include <thrift/TApplicationException.h>
+
+#include <limits>
+#include <boost/static_assert.hpp>
+
+
+namespace apache { namespace thrift { namespace protocol {
+
+void THeaderProtocol::resetProtocol() {
+ if (proto_ && protoId_ == trans_->getProtocolId()) {
+ return;
+ }
+
+ protoId_ = trans_->getProtocolId();
+
+ switch (protoId_) {
+ case T_BINARY_PROTOCOL:
+ proto_ = boost::shared_ptr<TProtocol>(
+ new TBinaryProtocolT<THeaderTransport>(trans_));
+ break;
+
+ case T_COMPACT_PROTOCOL:
+ proto_ = boost::shared_ptr<TProtocol>(
+ new TCompactProtocolT<THeaderTransport>(trans_));
+ break;
+
+ default:
+ throw TApplicationException(TApplicationException::INVALID_PROTOCOL,
+ "Unknown protocol requested");
+ }
+}
+
+uint32_t THeaderProtocol::writeMessageBegin(const std::string& name,
+ const TMessageType messageType,
+ const int32_t seqId) {
+ resetProtocol(); // Reset in case we changed protocols
+ trans_->setSequenceNumber(seqId);
+ return proto_->writeMessageBegin(name, messageType, seqId);
+}
+
+uint32_t THeaderProtocol::writeMessageEnd() {
+ return proto_->writeMessageEnd();
+}
+
+uint32_t THeaderProtocol::writeStructBegin(const char* name) {
+ return proto_->writeStructBegin(name);
+}
+
+uint32_t THeaderProtocol::writeStructEnd() {
+ return proto_->writeStructEnd();
+}
+
+uint32_t THeaderProtocol::writeFieldBegin(const char* name,
+ const TType fieldType,
+ const int16_t fieldId) {
+ return proto_->writeFieldBegin(name, fieldType, fieldId);
+}
+
+uint32_t THeaderProtocol::writeFieldEnd() {
+ return proto_->writeFieldEnd();
+}
+
+uint32_t THeaderProtocol::writeFieldStop() {
+ return proto_->writeFieldStop();
+}
+
+uint32_t THeaderProtocol::writeMapBegin(const TType keyType,
+ const TType valType,
+ const uint32_t size) {
+ return proto_->writeMapBegin(keyType, valType, size);
+}
+
+uint32_t THeaderProtocol::writeMapEnd() {
+ return proto_->writeMapEnd();
+}
+
+uint32_t THeaderProtocol::writeListBegin(const TType elemType,
+ const uint32_t size) {
+ return proto_->writeListBegin(elemType, size);
+}
+
+uint32_t THeaderProtocol::writeListEnd() {
+ return proto_->writeListEnd();
+}
+
+uint32_t THeaderProtocol::writeSetBegin(const TType elemType,
+ const uint32_t size) {
+ return proto_->writeSetBegin(elemType, size);
+}
+
+uint32_t THeaderProtocol::writeSetEnd() {
+ return proto_->writeSetEnd();
+}
+
+uint32_t THeaderProtocol::writeBool(const bool value) {
+ return proto_->writeBool(value);
+}
+
+uint32_t THeaderProtocol::writeByte(const int8_t byte) {
+ return proto_->writeByte(byte);
+}
+
+uint32_t THeaderProtocol::writeI16(const int16_t i16) {
+ return proto_->writeI16(i16);
+}
+
+uint32_t THeaderProtocol::writeI32(const int32_t i32) {
+ return proto_->writeI32(i32);
+}
+
+uint32_t THeaderProtocol::writeI64(const int64_t i64) {
+ return proto_->writeI64(i64);
+}
+
+uint32_t THeaderProtocol::writeDouble(const double dub) {
+ return proto_->writeDouble(dub);
+}
+
+uint32_t THeaderProtocol::writeString(const std::string& str) {
+ return proto_->writeString(str);
+}
+
+uint32_t THeaderProtocol::writeBinary(const std::string& str) {
+ return proto_->writeBinary(str);
+}
+
+/**
+ * Reading functions
+ */
+
+uint32_t THeaderProtocol::readMessageBegin(std::string& name,
+ TMessageType& messageType,
+ int32_t& seqId) {
+ // Read the next frame, and change protocols if needed
+ try {
+ trans_->resetProtocol();
+ resetProtocol();
+ } catch (const TApplicationException& ex) {
+ writeMessageBegin("", T_EXCEPTION, 0);
+ ex.write((TProtocol*)this);
+ writeMessageEnd();
+ trans_->flush();
+ }
--- End diff --
The above try / catch block effectively swallows the exception after writing out a response. I would expect it to re-throw the exception, or at least to do something like return 0. That way calling code can know that something is wrong, and that a new message shouldn't be read.
Alternatively, if there is a strong reason to not propagate that information, a supporting comment would do.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507848
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -547,12 +553,18 @@ void TNonblockingServer::TConnection::transition() {
case APP_READ_REQUEST:
// We are done reading the request, package the read buffer into transport
// and get back some data from the dispatch function
- inputTransport_->resetBuffer(readBuffer_, readBufferPos_);
- outputTransport_->resetBuffer();
- // Prepend four bytes of blank space to the buffer so we can
- // write the frame size there later.
- outputTransport_->getWritePtr(4);
- outputTransport_->wroteBytes(4);
+ if (!server_->getOutputProtocolFactory()) {
+ inputTransport_->resetBuffer(readBuffer_, readBufferPos_);
+ outputTransport_->resetBuffer();
+ } else {
+ inputTransport_->resetBuffer(readBuffer_ + 4, readBufferPos_ - 4);
--- End diff --
I believe this is the 'legacy' branch. Is there a compelling reason to change this code from how it was? I'm not sure why the +/- 4 is necessary now, but for some reason wasn't necessary before. Maybe this isn't a legacy branch and I'm just misunderstanding things?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507993
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
+ int16_t numTransforms;
+ ptr += readVarint16(ptr, &numTransforms, headerBoundary);
+
+ uint16_t macSz = 0;
--- End diff --
This appears unused... except as a more convoluted no-op with 'sz -= macSz;'. I'm guessing that the 'mac' (message authentication code?) feature was dropped in this push.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-70496980
I'll review this... it's going to take a couple of days though.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23307653
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
+
+enum CLIENT_TYPE {
+ THRIFT_HEADER_CLIENT_TYPE = 0,
+ THRIFT_FRAMED_DEPRECATED = 1,
+ THRIFT_UNFRAMED_DEPRECATED = 2,
+ THRIFT_UNKNOWN_CLIENT_TYPE = 4,
+};
+
+namespace apache { namespace thrift { namespace transport {
+
+using apache::thrift::protocol::T_COMPACT_PROTOCOL;
+
+/**
+ * Header transport. All writes go into an in-memory buffer until flush is
+ * called, at which point the transport writes the length of the entire
+ * binary chunk followed by the data payload. This allows the receiver on the
+ * other end to always do fixed-length reads.
+ *
+ * Subclass TFramedTransport because most of the read/write methods are similar
+ * and need similar buffers. Major changes are readFrame & flush.
+ */
+class THeaderTransport
+ : public TVirtualTransport<THeaderTransport, TFramedTransport> {
+ public:
+
+ static const int DEFAULT_BUFFER_SIZE = 512u;
+
+ /// Use default buffer sizes.
+ explicit THeaderTransport(const boost::shared_ptr<TTransport> transport)
+ : transport_(transport)
+ , outTransport_(transport)
+ , protoId(T_COMPACT_PROTOCOL)
+ , clientType(THRIFT_HEADER_CLIENT_TYPE)
+ , seqId(0)
+ , flags(0)
+ , tBufSize_(0)
+ , tBuf_(NULL)
+ {
+ initBuffers();
+ }
+
+ THeaderTransport(const boost::shared_ptr<TTransport> inTransport,
+ const boost::shared_ptr<TTransport> outTransport)
+ : transport_(inTransport)
+ , outTransport_(outTransport)
+ , protoId(T_COMPACT_PROTOCOL)
+ , clientType(THRIFT_HEADER_CLIENT_TYPE)
+ , seqId(0)
+ , flags(0)
+ , tBufSize_(0)
+ , tBuf_(NULL)
+ {
+ initBuffers();
+ }
+
+ void open() {
+ transport_->open();
+ }
+
+ bool isOpen() {
+ return transport_->isOpen();
+ }
+
+ bool peek() {
+ return (this->rBase_ < this->rBound_) || transport_->peek();
+ }
+
+ void close() {
+ flush();
+ transport_->close();
+ }
+
+ virtual uint32_t readSlow(uint8_t* buf, uint32_t len);
+ virtual uint32_t readAll(uint8_t* buf, uint32_t len);
+ virtual void flush();
+
+ void resizeTransformBuffer();
+
+ boost::shared_ptr<TTransport> getUnderlyingTransport() {
+ return transport_;
+ }
+
+ /*
+ * TVirtualTransport provides a default implementation of readAll().
+ * We want to use the TBufferBase version instead.
+ */
+ using TBufferBase::readAll;
+
+ uint16_t getProtocolId() const;
+ void setProtocolId(uint16_t protoId) { this->protoId = protoId; }
+
+ void resetProtocol();
+
+ /**
+ * We know we got a packet in header format here, try to parse the header
+ *
+ * @param headerSize size of the header portion
+ * @param sz Size of the whole message, including header
+ */
+ void readHeaderFormat(uint16_t headerSize, uint32_t sz);
+
+ /**
+ * Untransform the data based on the recieved header flags
+ * On conclusion of function, setReadBuffer is called with the
+ * untransformed data.
+ *
+ * @param ptr ptr to data
+ * @param size of data
+ */
+ void untransform(uint8_t* ptr, uint32_t sz);
+
+ /**
+ * Transform the data based on our write transform flags
+ * At conclusion of function the write buffer is set to the
+ * transformed data.
+ *
+ * @param ptr Ptr to data to transform
+ * @param sz Size of data buffer
+ */
+ void transform(uint8_t* ptr, uint32_t sz);
+
+ uint16_t getNumTransforms() const {
+ int trans = writeTrans_.size();
+ return trans;
+ }
+
+ void setTransform(uint16_t transId) { writeTrans_.push_back(transId); }
+
+ // Info headers
+
+ typedef std::map<std::string, std::string> StringToStringMap;
+
+ // these work with write headers
+ void setHeader(const std::string& key, const std::string& value);
+
+ void clearHeaders();
+
+ StringToStringMap& getWriteHeaders() {
+ return writeHeaders_;
+ }
+
+ // these work with read headers
+ const StringToStringMap& getHeaders() const {
+ return readHeaders_;
+ }
+
+ // accessors for seqId
+ int32_t getSequenceNumber() const { return seqId; }
+ void setSequenceNumber(int32_t seqId) { this->seqId = seqId; }
+
+ enum TRANSFORMS {
+ ZLIB_TRANSFORM = 0x01,
+ };
+
+ protected:
+
+ std::bitset<CLIENT_TYPES_LEN> supported_clients;
+
+ void initSupportedClients(std::bitset<CLIENT_TYPES_LEN> const*);
+
+ /**
+ * Reads a frame of input from the underlying stream.
+ *
+ * Returns true if a frame was read successfully, or false on EOF.
+ * (Raises a TTransportException if EOF occurs after a partial frame.)
+ */
+ bool readFrame(uint32_t minFrameSize);
+
+ void allocateReadBuffer(uint32_t sz);
+ uint32_t getWriteBytes();
+
+ void initBuffers() {
+ setReadBuffer(NULL, 0);
+ setWriteBuffer(wBuf_.get(), wBufSize_);
+ }
+
+ boost::shared_ptr<TTransport> transport_;
+ boost::shared_ptr<TTransport> outTransport_;
+
+ // 0 and 16th bits must be 0 to differentiate from framed & unframed
+ static const uint32_t HEADER_MAGIC = 0x0FFF0000;
+ static const uint32_t HEADER_MASK = 0xFFFF0000;
+ static const uint32_t FLAGS_MASK = 0x0000FFFF;
+
+ static const uint32_t MAX_FRAME_SIZE = 0x3FFFFFFF;
+
+ int16_t protoId;
+ uint16_t clientType;
+ uint32_t seqId;
+ uint16_t flags;
+
+ std::vector<uint16_t> readTrans_;
+ std::vector<uint16_t> writeTrans_;
+
+ // Map to use for headers
+ StringToStringMap readHeaders_;
+ StringToStringMap writeHeaders_;
+
+ /**
+ * Returns the maximum number of bytes that write k/v headers can take
+ */
+ size_t getMaxWriteHeadersSize() const;
+
+ struct infoIdType {
+ enum idType {
+ // start at 1 to avoid confusing header padding for an infoId
+ KEYVALUE = 1,
+ END // signal the end of infoIds we can handle
+ };
+ };
+
+ // Buffers to use for transform processing
+ uint32_t tBufSize_;
+ boost::scoped_array<uint8_t> tBuf_;
+
+ void readString(uint8_t* &ptr, /* out */ std::string &str,
+ uint8_t const* headerBoundary);
+
+ void writeString(uint8_t* &ptr, const std::string& str);
+
+ // Varint utils
+ /**
+ * Read an i16 from the wire as a varint. The MSB of each byte is set
+ * if there is another byte to follow. This can read up to 3 bytes.
+ */
+ uint32_t readVarint16(uint8_t const* ptr, int16_t* i16,
+ uint8_t const* boundary);
+
+ /**
+ * Read an i32 from the wire as a varint. The MSB of each byte is set
+ * if there is another byte to follow. This can read up to 5 bytes.
+ */
+ uint32_t readVarint32(uint8_t const* ptr, int32_t* i32,
+ uint8_t const* boundary);
+
+ /**
+ * Write an i32 as a varint. Results in 1-5 bytes on the wire.
+ */
+ uint32_t writeVarint32(uint32_t n, uint8_t* pkt);
--- End diff --
If this is writing an i32... 'n' should be an int32_t instead of a uint32_t.
Similar commends on writeVarint16.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23508204
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
+ int16_t numTransforms;
+ ptr += readVarint16(ptr, &numTransforms, headerBoundary);
+
+ uint16_t macSz = 0;
+
+ // For now all transforms consist of only the ID, not data.
+ for (int i = 0; i < numTransforms; i++) {
+ int32_t transId;
+ ptr += readVarint32(ptr, &transId, headerBoundary);
+
+ readTrans_.push_back(transId);
+ }
+
+ // Info headers
+ while (ptr < headerBoundary) {
+ uint32_t infoId;
+ ptr += readVarint32(ptr, (int32_t*)&infoId, headerBoundary);
+
+ if (infoId == 0) {
+ // header padding
+ break;
+ }
+ if (infoId >= infoIdType::END) {
+ // cannot handle infoId
+ break;
+ }
+ switch (infoId) {
+ case infoIdType::KEYVALUE:
+ // Process key-value headers
+ uint32_t numKVHeaders;
+ ptr += readVarint32(ptr, (int32_t*)&numKVHeaders, headerBoundary);
+ // continue until we reach (padded) end of packet
+ while (numKVHeaders-- && ptr < headerBoundary) {
+ // format: key; value
+ // both: length (varint32); value (string)
+ string key, value;
+ readString(ptr, key, headerBoundary);
+ // value
+ readString(ptr, value, headerBoundary);
+ // save to headers
+ readHeaders_[key] = value;
+ }
+ break;
+ }
+ }
+
+ // Don't include the mac in the data to untransform
+ sz -= macSz;
+
+ // Untransform the data section. rBuf will contain result.
+ untransform(data, sz - (data - rBuf_.get())); // ignore header in size calc
+}
+
+void THeaderTransport::untransform(uint8_t* ptr, uint32_t sz) {
+ // Update the transform buffer size if needed
+ resizeTransformBuffer();
+
+ for (vector<uint16_t>::const_iterator it = readTrans_.begin();
+ it != readTrans_.end(); ++it) {
+ const uint16_t transId = *it;
+
+ if (transId == ZLIB_TRANSFORM) {
+ z_stream stream;
+ int err;
+
+ stream.next_in = ptr;
+ stream.avail_in = sz;
+
+ // Setting these to 0 means use the default free/alloc functions
+ stream.zalloc = (alloc_func)0;
+ stream.zfree = (free_func)0;
+ stream.opaque = (voidpf)0;
+ err = inflateInit(&stream);
+ if (err != Z_OK) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflateInit");
+ }
+ stream.next_out = tBuf_.get();
+ stream.avail_out = tBufSize_;
+ err = inflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END || stream.avail_out == 0) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflate");
+ }
+ sz = stream.total_out;
+
+ err = inflateEnd(&stream);
+ if (err != Z_OK) {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Error while zlib deflateEnd");
+ }
+
+ memcpy(ptr, tBuf_.get(), sz);
+ } else {
+ throw TApplicationException(TApplicationException::MISSING_RESULT,
+ "Unknown transform");
+ }
+ }
+
+ setReadBuffer(ptr, sz);
+}
+
+/**
+ * We may have updated the wBuf size, update the tBuf size to match.
+ * Should be called in transform.
+ *
+ * The buffer should be slightly larger than write buffer size due to
+ * compression transforms (that may slightly grow on small frame sizes)
+ */
+void THeaderTransport::resizeTransformBuffer() {
+ if (tBufSize_ < wBufSize_ + DEFAULT_BUFFER_SIZE) {
+ uint32_t new_size = wBufSize_ + DEFAULT_BUFFER_SIZE;
+ uint8_t* new_buf = new uint8_t[new_size];
+ tBuf_.reset(new_buf);
+ tBufSize_ = new_size;
+ }
+}
+
+void THeaderTransport::transform(uint8_t* ptr, uint32_t sz) {
+ // Update the transform buffer size if needed
+ resizeTransformBuffer();
+
+ for (vector<uint16_t>::const_iterator it = writeTrans_.begin();
+ it != writeTrans_.end(); ++it) {
+ const uint16_t transId = *it;
+
+ if (transId == ZLIB_TRANSFORM) {
+ z_stream stream;
+ int err;
+
+ stream.next_in = ptr;
+ stream.avail_in = sz;
+
+ stream.zalloc = (alloc_func)0;
+ stream.zfree = (free_func)0;
+ stream.opaque = (voidpf)0;
+ err = deflateInit(&stream, Z_DEFAULT_COMPRESSION);
+ if (err != Z_OK) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Error while zlib deflateInit");
+ }
+ stream.next_out = tBuf_.get();
+ stream.avail_out = tBufSize_;
+ err = deflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END || stream.avail_out == 0) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Error while zlib deflate");
+ }
+ sz = stream.total_out;
+
+ err = deflateEnd(&stream);
+ if (err != Z_OK) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Error while zlib deflateEnd");
+ }
+
+ memcpy(ptr, tBuf_.get(), sz);
+ } else {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Unknown transform");
+ }
+ }
+
+ wBase_ = wBuf_.get() + sz;
+}
+
+void THeaderTransport::resetProtocol() {
+ // Set to anything except HTTP type so we don't flush again
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+
+ // Read the header and decide which protocol to go with
+ readFrame(0);
+}
+
+uint32_t THeaderTransport::getWriteBytes() {
+ return wBase_ - wBuf_.get();
+}
+
+/**
+ * Writes a string to a byte buffer, as size (varint32) + string (non-null
+ * terminated)
+ * Automatically advances ptr to after the written portion
+ */
+void THeaderTransport::writeString(uint8_t* &ptr, const string& str) {
+ uint32_t strLen = str.length();
+ ptr += writeVarint32(strLen, ptr);
+ memcpy(ptr, str.c_str(), strLen); // no need to write \0
+ ptr += strLen;
+}
+
+void THeaderTransport::setHeader(const string& key, const string& value) {
+ writeHeaders_[key] = value;
+}
+
+size_t THeaderTransport::getMaxWriteHeadersSize() const {
+ size_t maxWriteHeadersSize = 0;
+ THeaderTransport::StringToStringMap::const_iterator it;
+ for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
+ // add sizes of key and value to maxWriteHeadersSize
+ // 2 varints32 + the strings themselves
+ maxWriteHeadersSize += 5 + 5 + (it->first).length() +
+ (it->second).length();
+ }
+ return maxWriteHeadersSize;
+}
+
+void THeaderTransport::clearHeaders() {
+ writeHeaders_.clear();
+}
+
+void THeaderTransport::flush() {
+ // Write out any data waiting in the write buffer.
+ uint32_t haveBytes = getWriteBytes();
+
+ if (clientType == THRIFT_HEADER_CLIENT_TYPE) {
+ transform(wBuf_.get(), haveBytes);
+ haveBytes = getWriteBytes(); // transform may have changed the size
+ }
+
+ // Note that we reset wBase_ prior to the underlying write
+ // to ensure we're in a sane state (i.e. internal buffer cleaned)
+ // if the underlying write throws up an exception
+ wBase_ = wBuf_.get();
+
+ if (haveBytes > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Attempting to send frame that is too large");
+ }
+
+ if (clientType == THRIFT_HEADER_CLIENT_TYPE) {
+ // header size will need to be updated at the end because of varints.
+ // Make it big enough here for max varint size, plus 4 for padding.
+ int headerSize = (2 + getNumTransforms()) * 5 + 4;
--- End diff --
a lot of magic numbers sprinkled about...
At least '5' should be easy enough to define as THRIFT_MAX_VARINT32_BYTES (or something similar)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r25187923
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
+
+enum CLIENT_TYPE {
+ THRIFT_HEADER_CLIENT_TYPE = 0,
+ THRIFT_FRAMED_DEPRECATED = 1,
+ THRIFT_UNFRAMED_DEPRECATED = 2,
+ THRIFT_UNKNOWN_CLIENT_TYPE = 4,
+};
+
+namespace apache { namespace thrift { namespace transport {
+
+using apache::thrift::protocol::T_COMPACT_PROTOCOL;
+
+/**
+ * Header transport. All writes go into an in-memory buffer until flush is
+ * called, at which point the transport writes the length of the entire
+ * binary chunk followed by the data payload. This allows the receiver on the
+ * other end to always do fixed-length reads.
+ *
+ * Subclass TFramedTransport because most of the read/write methods are similar
--- End diff --
Having a TFramedTransport as a member is harder than it seems - THeaderTransport should probably inherit from TBufferedTransport, but then we'd have two TBufferedTransports, one for header and one for framed, to make work together somehow. I could just copy in TFramedTransport::readSlow I guess
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2423: Facebook's THeader protocol and ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/357
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23505627
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
+
+enum CLIENT_TYPE {
+ THRIFT_HEADER_CLIENT_TYPE = 0,
+ THRIFT_FRAMED_DEPRECATED = 1,
+ THRIFT_UNFRAMED_DEPRECATED = 2,
+ THRIFT_UNKNOWN_CLIENT_TYPE = 4,
+};
+
+namespace apache { namespace thrift { namespace transport {
+
+using apache::thrift::protocol::T_COMPACT_PROTOCOL;
+
+/**
+ * Header transport. All writes go into an in-memory buffer until flush is
+ * called, at which point the transport writes the length of the entire
+ * binary chunk followed by the data payload. This allows the receiver on the
+ * other end to always do fixed-length reads.
+ *
+ * Subclass TFramedTransport because most of the read/write methods are similar
--- End diff --
I don't think I like subclassing TFramedTransport. Some of your members call TBufferBase methods, while others call TFramedTransport.
In addition, by subclassing TFramedTransport, you allow THeaderTransports to be passed to functions expecting a TFramedTransport. That could cause some hard to diagnose bugs, as TFramedTransport's wire layout is different from THeaderTransport's.
If you want to continue to get code reuse from TFramedTransport, then that's great, and you can do so by using containment instead of inheritance. Make a member TFramedTransport, and forward calls to the member. In the case where you want to call into TBufferBase versions of methods, you can do so like this:
framedTransport.TBufferBase::readAll(buf, len);
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23559637
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -547,12 +553,18 @@ void TNonblockingServer::TConnection::transition() {
case APP_READ_REQUEST:
// We are done reading the request, package the read buffer into transport
// and get back some data from the dispatch function
- inputTransport_->resetBuffer(readBuffer_, readBufferPos_);
- outputTransport_->resetBuffer();
- // Prepend four bytes of blank space to the buffer so we can
- // write the frame size there later.
- outputTransport_->getWritePtr(4);
- outputTransport_->wroteBytes(4);
+ if (!server_->getOutputProtocolFactory()) {
+ inputTransport_->resetBuffer(readBuffer_, readBufferPos_);
+ outputTransport_->resetBuffer();
+ } else {
+ inputTransport_->resetBuffer(readBuffer_ + 4, readBufferPos_ - 4);
--- End diff --
THeader expects the first four bytes of framing to still be in the message. Nonblocking server previously always stripped it off
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2423: Facebook's THeader protocol and ...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-153356845
There were several build issues.
I'll commit the fixed version shortly after #677.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507973
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
+ int16_t numTransforms;
+ ptr += readVarint16(ptr, &numTransforms, headerBoundary);
+
+ uint16_t macSz = 0;
+
+ // For now all transforms consist of only the ID, not data.
+ for (int i = 0; i < numTransforms; i++) {
+ int32_t transId;
+ ptr += readVarint32(ptr, &transId, headerBoundary);
+
+ readTrans_.push_back(transId);
+ }
+
+ // Info headers
+ while (ptr < headerBoundary) {
+ uint32_t infoId;
+ ptr += readVarint32(ptr, (int32_t*)&infoId, headerBoundary);
+
+ if (infoId == 0) {
+ // header padding
+ break;
+ }
+ if (infoId >= infoIdType::END) {
+ // cannot handle infoId
+ break;
+ }
+ switch (infoId) {
+ case infoIdType::KEYVALUE:
+ // Process key-value headers
+ uint32_t numKVHeaders;
--- End diff --
The type mismatch with the following readVarInt32 is likely to make Java unhappy. Remember that Java doesn't have unsigned types. Data on the wire should generally be treated as signed in order to appease that language.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23305046
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
--- End diff --
very minor... this should use < > instead of " ".
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23305256
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -0,0 +1,308 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_TRANSPORT_THEADERTRANSPORT_H_
+#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
+
+#include <tr1/functional>
+
+#include <thrift/protocol/TBinaryProtocol.h>
+#include <thrift/protocol/TProtocolTypes.h>
+#include <thrift/transport/TBufferTransports.h>
+#include <thrift/transport/TTransport.h>
+#include <thrift/transport/TVirtualTransport.h>
+
+#include <bitset>
+#include "boost/scoped_array.hpp"
+#include <pwd.h>
+#include <unistd.h>
+
+// Don't include the unknown client.
+#define CLIENT_TYPES_LEN 4
+
+enum CLIENT_TYPE {
+ THRIFT_HEADER_CLIENT_TYPE = 0,
+ THRIFT_FRAMED_DEPRECATED = 1,
+ THRIFT_UNFRAMED_DEPRECATED = 2,
+ THRIFT_UNKNOWN_CLIENT_TYPE = 4,
+};
+
+namespace apache { namespace thrift { namespace transport {
+
+using apache::thrift::protocol::T_COMPACT_PROTOCOL;
+
+/**
+ * Header transport. All writes go into an in-memory buffer until flush is
+ * called, at which point the transport writes the length of the entire
+ * binary chunk followed by the data payload. This allows the receiver on the
+ * other end to always do fixed-length reads.
+ *
+ * Subclass TFramedTransport because most of the read/write methods are similar
+ * and need similar buffers. Major changes are readFrame & flush.
+ */
+class THeaderTransport
+ : public TVirtualTransport<THeaderTransport, TFramedTransport> {
+ public:
+
+ static const int DEFAULT_BUFFER_SIZE = 512u;
+
+ /// Use default buffer sizes.
+ explicit THeaderTransport(const boost::shared_ptr<TTransport> transport)
--- End diff --
pass by const &. Your other ctor also needs some &s sprinkled on its args.
If we were using C++11 everywhere, then _maybe_ we would pass by value and then std::move into the member variable... but we aren't using C++11 everywhere.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23560447
--- 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) {
+ // unframed
+ clientType = THRIFT_UNFRAMED_DEPRECATED;
+ memcpy(rBuf_.get(), &szN, sizeof(szN));
+ if (minFrameSize > 4) {
+ transport_->readAll(rBuf_.get() + 4, minFrameSize - 4);
+ setReadBuffer(rBuf_.get(), minFrameSize);
+ } else {
+ setReadBuffer(rBuf_.get(), 4);
+ }
+ } else {
+ // Could be header format or framed. Check next uint32
+ uint32_t magic_n;
+ uint32_t magic;
+
+ if (sz > MAX_FRAME_SIZE) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too large");
+ }
+
+ allocateReadBuffer(sz);
+
+ // We can use readAll here, because it would be an invalid frame otherwise
+ transport_->readAll(reinterpret_cast<uint8_t*>(&magic_n), sizeof(magic_n));
+ memcpy(rBuf_.get(), &magic_n, sizeof(magic_n));
+ magic = ntohl(magic_n);
+
+ if ((magic & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
+ // framed
+ clientType = THRIFT_FRAMED_DEPRECATED;
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+ setReadBuffer(rBuf_.get(), sz);
+ } else if (HEADER_MAGIC == (magic & HEADER_MASK)) {
+ if (sz < 10) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header transport frame is too small");
+ }
+
+ transport_->readAll(rBuf_.get() + 4, sz - 4);
+
+ // header format
+ clientType = THRIFT_HEADER_CLIENT_TYPE;
+ // flags
+ flags = magic & FLAGS_MASK;
+ // seqId
+ uint32_t seqId_n;
+ memcpy(&seqId_n, rBuf_.get() + 4, sizeof(seqId_n));
+ seqId = ntohl(seqId_n);
+ // header size
+ uint16_t headerSize_n;
+ memcpy(&headerSize_n, rBuf_.get() + 8, sizeof(headerSize_n));
+ uint16_t headerSize = ntohs(headerSize_n);
+ setReadBuffer(rBuf_.get(), sz);
+ readHeaderFormat(headerSize, sz);
+ } else {
+ clientType = THRIFT_UNKNOWN_CLIENT_TYPE;
+ throw TTransportException(TTransportException::BAD_ARGS,
+ "Could not detect client transport type");
+ }
+ }
+
+
+ return true;
+}
+
+
+/**
+ * Reads a string from ptr, taking care not to reach headerBoundary
+ * Advances ptr on success
+ *
+ * @param str output string
+ * @throws CORRUPTED_DATA if size of string exceeds boundary
+ */
+void THeaderTransport::readString(uint8_t* &ptr, /* out */ string &str,
+ uint8_t const* headerBoundary) {
+ int32_t strLen;
+
+ uint32_t bytes = readVarint32(ptr, &strLen, headerBoundary);
+ if (ptr + strLen > headerBoundary) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Info header length exceeds header size");
+ }
+ ptr += bytes;
+ str.assign(reinterpret_cast<const char*>(ptr), strLen);
+ ptr += strLen;
+}
+
+void THeaderTransport::readHeaderFormat(uint16_t headerSize, uint32_t sz) {
+ readTrans_.clear(); // Clear out any previous transforms.
+ readHeaders_.clear(); // Clear out any previous headers.
+
+ // skip over already processed magic(4), seqId(4), headerSize(2)
+ uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
+ headerSize *= 4;
+ const uint8_t* const headerBoundary = ptr + headerSize;
+ if (headerSize > sz) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is larger than frame");
+ }
+ uint8_t* data = ptr + headerSize;
+ ptr += readVarint16(ptr, &protoId, headerBoundary);
--- End diff --
I could see arrays of varints being faster, since you would reduce data cache pressure. I don't have specific numbers though, and numbers rightfully beat out thought-experiments.
Protocolbufs always does the varint thing. One of the main designers of protocolbufs went on to make this: https://capnproto.org/
So he's going with a mem-mappable layout. That would be awesome, but would require some pretty significant rearchitecture.
Regardless, I won't hold up this commit over varints. I'm still skeptical, but not skeptical enough to block progress on something that adds as much value as THeader.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23507254
--- 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) {
--- End diff --
not super-thrilled with this name, as sometimes it allocates, and sometimes it doesn't. Maybe 'ensureReadBufferAllocated'? 'reserveReadBuffer'?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Facebook's THeader protocol and transport for...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/357#discussion_r23333773
--- Diff: lib/cpp/src/thrift/protocol/TProtocolTypes.h ---
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#ifndef THRIFT_PROTOCOL_TPROTOCOLTYPES_H_
+#define THRIFT_PROTOCOL_TPROTOCOLTYPES_H_ 1
+
+namespace apache { namespace thrift { namespace protocol {
+
+enum PROTOCOL_TYPES {
+ T_BINARY_PROTOCOL = 0,
+ T_JSON_PROTOCOL = 1,
+ T_COMPACT_PROTOCOL = 2,
+};
--- End diff --
True words. There are a bunch of candidates.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2423: Facebook's THeader protocol and ...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/357#issuecomment-153061344
What happened to this ?
Although it does not cleanly apply at all anymore, with "git merge" it is still straightforward to merge.
If there isn't any concern, I'll merge this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---