You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/06/23 02:56:46 UTC

[GitHub] [thrift] zeshuai007 opened a new pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class

zeshuai007 opened a new pull request #2185:
URL: https://github.com/apache/thrift/pull/2185


   
   <!-- Explain the changes in the pull request below: -->
     
   
   <!-- We recommend you review the checklist/tips before submitting a pull request. -->
   
   - [X] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket?  (not required for trivial changes)
   - [X] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
   - [X] Did you squash your changes to a single commit?  (not required, but preferred)
   - [X] Did you do your best to avoid breaking changes?  If one was needed, did you label the Jira ticket with "Breaking-Change"?
   - [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
   
   <!--
     The Contributing Guide at:
     https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
     has more details and tips for committing properly.
   -->
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-768862691


   Is this now dead or just replaced by another PR?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] dzhang-b commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
dzhang-b commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-842520102


   Hi, there is no way to pass the Tconfiguration class into a TBufferedTransportFactory to produce a TBufferedTransport with correct config.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] zeshuai007 commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-660004094


   ```
   tardir=thrift-0.14.0 && tar --hard-dereference --format=ustar -chf - "$tardir" | eval GZIP= gzip --best -c >thrift-0.14.0.tar.gz
   make[1]: Leaving directory '/thrift/src'
   if test -d "thrift-0.14.0"; then find "thrift-0.14.0" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "thrift-0.14.0" || { sleep 5 && rm -rf "thrift-0.14.0"; }; else :; fi
   tar xvf thrift-*.tar.gz
   thrift-0.14.0/
   thrift-0.14.0/README.md
   ......
   ```
   
   @Jens-G , I look through the CI log , I found that `thrift-*.tar.gz` doesn't  contains the file which I new added, but I don't know how to cram them into  `thrift-*.tar.gz`, and why this CI doesn't use the content from `git clone` directly.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on a change in pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#discussion_r450493453



##########
File path: lib/cpp/src/thrift/TConfiguration.h
##########
@@ -0,0 +1,55 @@
+/*
+ * 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_TCONFIGURATION_H
+#define THRIFT_TCONFIGURATION_H
+
+namespace apache {
+namespace thrift {
+
+class TConfiguration
+{
+public:
+   TConfiguration(int maxMessageSize = 100 * 1024 * 1024, 
+                  int maxFrameSize = 16384000, int recursionLimit = 64)
+     : maxMessageSize_(maxMessageSize), maxFrameSize_(maxFrameSize), recursionLimit_(recursionLimit) {}
+
+    const int DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024;
+    const int DEFAULT_MAX_FRAME_SIZE = 16384000;      // this value is used consistently across all Thrift libraries
+    const int DEFAULT_RECURSION_DEPTH = 64;
+

Review comment:
       We have the same values defined twice. Why cxan't we use the constants in the CTOR decl?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] zeshuai007 commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-658595859


   > [3/333] [BISON][thrifty] Building parser with bison 3.0.4�[K
   > [4/333] Building CXX object lib/cpp/CM...ansport/TNonblockingServerSocket.cpp.o�[K
   > FAILED: lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o
   > /usr/bin/g++ -DBOOST_ALL_DYN_LINK -DBOOST_TEST_DYN_LINK -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dthriftnb_EXPORTS -Ilib/cpp -I../lib/cpp -I. -I../lib/cpp/src -Wall -Wextra -fmax-errors=1 -O2 -g -DNDEBUG -fPIC -std=c++11 -MD -MT lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -MF lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o.d -o lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -c ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp
   > In file included from ../lib/cpp/src/thrift/transport/TSocket.h:25:0,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerTransport.h:23,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.h:23,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp:48:
   > ../lib/cpp/src/thrift/transport/TTransport.h:24:10: fatal error: thrift/TConfiguration.h: No such file or directory
   > #include <thrift/TConfiguration.h>
   > ^~~~~~~~~~~~~~~~~~~~~~~~~
   > compilation terminated.
   
   Hi @Jens-G ,  I am very confused about this problem. File `thrift/TConfiguration.h` does exist, and most of the CI is passed except this.
   Any suggestions from you?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-768868746


   https://github.com/apache/thrift/pull/2319


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] taras-protsiv commented on a change in pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
taras-protsiv commented on a change in pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#discussion_r565037584



##########
File path: lib/cpp/src/thrift/transport/TTransport.h
##########
@@ -238,11 +248,87 @@ class TTransport {
    */
   virtual const std::string getOrigin() const { return "Unknown"; }
 
+  std::shared_ptr<TConfiguration> getConfiguration() { return configuration_; }
+
+  void setConfiguration(std::shared_ptr<TConfiguration> config) { 
+    if (config != nullptr) configuration_ = config; 
+  }
+
+  /**
+   * Updates RemainingMessageSize to reflect then known real message size (e.g. framed transport).
+   * Will throw if we already consumed too many bytes or if the new size is larger than allowed.
+   *
+   * @param size  real message size
+   */
+  void updateKnownMessageSize(long int size)
+  {
+    long int consumed = knownMessageSize_ - remainingMessageSize_;
+    resetConsumedMessageSize(size);
+    countConsumedMessageBytes(consumed);
+  }
+
+  /**
+   * Throws if there are not enough bytes in the input stream to satisfy a read of numBytes bytes of data
+   *
+   * @param numBytes  numBytes bytes of data
+   */
+  void checkReadBytesAvailable(long int numBytes)
+  {
+    if (remainingMessageSize_ < numBytes)
+      throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");
+  }
+
 protected:
+  std::shared_ptr<TConfiguration> configuration_;
+  long int remainingMessageSize_;
+  long int knownMessageSize_;
+
+  inline long int getRemainingMessageSize() { return remainingMessageSize_; }
+  inline void setRemainingMessageSize(long int remainingMessageSize) { remainingMessageSize_ = remainingMessageSize; }
+  inline int getMaxMessageSize() { return configuration_->getMaxMessageSize(); }
+  inline long int getKnownMessageSize() { return knownMessageSize_; }
+  void setKnownMessageSize(long int knownMessageSize) { knownMessageSize_ = knownMessageSize; }
+
+  /**  
+   * Resets RemainingMessageSize to the configured maximum
+   * 
+   *  @param newSize  configured size
+   */
+  void resetConsumedMessageSize(long newSize = -1)
+  {
+    // full reset 
+    if (newSize < 0)
+    {
+        knownMessageSize_ = getMaxMessageSize();
+        remainingMessageSize_ = getMaxMessageSize();
+        return;
+    }
+
+    // update only: message size can shrink, but not grow
+    if (newSize > knownMessageSize_)
+        throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");
+
+    knownMessageSize_ = newSize;
+    remainingMessageSize_ = newSize;
+  }
+
   /**
-   * Simple constructor.
+   * Consumes numBytes from the RemainingMessageSize.
+   * 
+   *  @param numBytes  Consumes numBytes
    */
-  TTransport() = default;
+  void countConsumedMessageBytes(long int numBytes)
+  {
+    if (remainingMessageSize_ >= numBytes)
+    {
+      remainingMessageSize_ -= numBytes;
+    }
+    else
+    {
+      remainingMessageSize_ = 0;
+      throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");

Review comment:
       throw raw pointer. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] zeshuai007 commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class

Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-651750901


   Hi @Jens-G , can you help me review this PR?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-655103250


   [3/333] [BISON][thrifty] Building parser with bison 3.0.4
   [4/333] Building CXX object lib/cpp/CM...ansport/TNonblockingServerSocket.cpp.o
   FAILED: lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o 
   /usr/bin/g++  -DBOOST_ALL_DYN_LINK -DBOOST_TEST_DYN_LINK -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dthriftnb_EXPORTS -Ilib/cpp -I../lib/cpp -I. -I../lib/cpp/src -Wall -Wextra -fmax-errors=1 -O2 -g -DNDEBUG -fPIC   -std=c++11 -MD -MT lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -MF lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o.d -o lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -c ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp
   In file included from ../lib/cpp/src/thrift/transport/TSocket.h:25:0,
                    from ../lib/cpp/src/thrift/transport/TNonblockingServerTransport.h:23,
                    from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.h:23,
                    from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp:48:
   ../lib/cpp/src/thrift/transport/TTransport.h:24:10: fatal error: thrift/TConfiguration.h: No such file or directory
    #include <thrift/TConfiguration.h>
             ^~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] zeshuai007 commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-661778105


   > [3/333] [BISON][thrifty] Building parser with bison 3.0.4�[K
   > [4/333] Building CXX object lib/cpp/CM...ansport/TNonblockingServerSocket.cpp.o�[K
   > FAILED: lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o
   > /usr/bin/g++ -DBOOST_ALL_DYN_LINK -DBOOST_TEST_DYN_LINK -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dthriftnb_EXPORTS -Ilib/cpp -I../lib/cpp -I. -I../lib/cpp/src -Wall -Wextra -fmax-errors=1 -O2 -g -DNDEBUG -fPIC -std=c++11 -MD -MT lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -MF lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o.d -o lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -c ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp
   > In file included from ../lib/cpp/src/thrift/transport/TSocket.h:25:0,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerTransport.h:23,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.h:23,
   > from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp:48:
   > ../lib/cpp/src/thrift/transport/TTransport.h:24:10: fatal error: thrift/TConfiguration.h: No such file or directory
   > #include <thrift/TConfiguration.h>
   > ^~~~~~~~~~~~~~~~~~~~~~~~~
   > compilation terminated.
   
   @Jens-G ,  Resolved.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G edited a comment on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-768862691


   @taras-protsiv mind to send a PR? Or at least file a JIRA ticket? Its not really helpful to leave it this way.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] dzhang-b commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
dzhang-b commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-842520102


   Hi, there is no way to pass the Tconfiguration class into a TBufferedTransportFactory to produce a TBufferedTransport with correct config.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-652623317


   > Hi @Jens-G , can you help me review this PR?
   
   Sure, need to find some time
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] zeshuai007 commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-658584750


   > Still broken
   > C:\projects\thrift\lib\cpp\src\thrift\transport\TPipe.cpp: In member function 'virtual uint32_t apache::thrift::transport::TWaitableNamedPipeImpl::read(uint8_t*, uint32_t)':
   > C:\projects\thrift\lib\cpp\src\thrift\transport\TPipe.cpp:157:3: error: 'checkReadBytesAvailable' was not declared in this scope
   > 157 | checkReadBytesAvailable(len);
   > | ^~~~~~~~~~~~~~~~~~~~~~~
   > compilation terminated due to -fmax-errors=1.
   > mingw32-make[2]: *** [lib\cpp\CMakeFiles\thrift.dir\build.make:483: lib/cpp/CMakeFiles/thrift.dir/src/thrift/transport/TPipe.cpp.obj] Error 1
   
   Resolved.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G closed pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2185:
URL: https://github.com/apache/thrift/pull/2185


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G edited a comment on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-768862691


   @taras-protsiv mind to send a PR?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-659662285


   Try "make dist", that's what CI does here. Is it reproducible?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] taras-protsiv commented on a change in pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
taras-protsiv commented on a change in pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#discussion_r565037516



##########
File path: lib/cpp/src/thrift/transport/TTransport.h
##########
@@ -238,11 +248,87 @@ class TTransport {
    */
   virtual const std::string getOrigin() const { return "Unknown"; }
 
+  std::shared_ptr<TConfiguration> getConfiguration() { return configuration_; }
+
+  void setConfiguration(std::shared_ptr<TConfiguration> config) { 
+    if (config != nullptr) configuration_ = config; 
+  }
+
+  /**
+   * Updates RemainingMessageSize to reflect then known real message size (e.g. framed transport).
+   * Will throw if we already consumed too many bytes or if the new size is larger than allowed.
+   *
+   * @param size  real message size
+   */
+  void updateKnownMessageSize(long int size)
+  {
+    long int consumed = knownMessageSize_ - remainingMessageSize_;
+    resetConsumedMessageSize(size);
+    countConsumedMessageBytes(consumed);
+  }
+
+  /**
+   * Throws if there are not enough bytes in the input stream to satisfy a read of numBytes bytes of data
+   *
+   * @param numBytes  numBytes bytes of data
+   */
+  void checkReadBytesAvailable(long int numBytes)
+  {
+    if (remainingMessageSize_ < numBytes)
+      throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");
+  }
+
 protected:
+  std::shared_ptr<TConfiguration> configuration_;
+  long int remainingMessageSize_;
+  long int knownMessageSize_;
+
+  inline long int getRemainingMessageSize() { return remainingMessageSize_; }
+  inline void setRemainingMessageSize(long int remainingMessageSize) { remainingMessageSize_ = remainingMessageSize; }
+  inline int getMaxMessageSize() { return configuration_->getMaxMessageSize(); }
+  inline long int getKnownMessageSize() { return knownMessageSize_; }
+  void setKnownMessageSize(long int knownMessageSize) { knownMessageSize_ = knownMessageSize; }
+
+  /**  
+   * Resets RemainingMessageSize to the configured maximum
+   * 
+   *  @param newSize  configured size
+   */
+  void resetConsumedMessageSize(long newSize = -1)
+  {
+    // full reset 
+    if (newSize < 0)
+    {
+        knownMessageSize_ = getMaxMessageSize();
+        remainingMessageSize_ = getMaxMessageSize();
+        return;
+    }
+
+    // update only: message size can shrink, but not grow
+    if (newSize > knownMessageSize_)
+        throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");

Review comment:
       throw raw pointer. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] taras-protsiv commented on a change in pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
taras-protsiv commented on a change in pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#discussion_r565037317



##########
File path: lib/cpp/src/thrift/transport/TTransport.h
##########
@@ -238,11 +248,87 @@ class TTransport {
    */
   virtual const std::string getOrigin() const { return "Unknown"; }
 
+  std::shared_ptr<TConfiguration> getConfiguration() { return configuration_; }
+
+  void setConfiguration(std::shared_ptr<TConfiguration> config) { 
+    if (config != nullptr) configuration_ = config; 
+  }
+
+  /**
+   * Updates RemainingMessageSize to reflect then known real message size (e.g. framed transport).
+   * Will throw if we already consumed too many bytes or if the new size is larger than allowed.
+   *
+   * @param size  real message size
+   */
+  void updateKnownMessageSize(long int size)
+  {
+    long int consumed = knownMessageSize_ - remainingMessageSize_;
+    resetConsumedMessageSize(size);
+    countConsumedMessageBytes(consumed);
+  }
+
+  /**
+   * Throws if there are not enough bytes in the input stream to satisfy a read of numBytes bytes of data
+   *
+   * @param numBytes  numBytes bytes of data
+   */
+  void checkReadBytesAvailable(long int numBytes)
+  {
+    if (remainingMessageSize_ < numBytes)
+      throw new TTransportException(TTransportException::END_OF_FILE, "MaxMessageSize reached");

Review comment:
       throw raw pointer. 
   It will not be caught by try {...} catch ( const TTransportException& ).
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2185: THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (cpp)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2185:
URL: https://github.com/apache/thrift/pull/2185#issuecomment-657271594


   Still broken
   C:\projects\thrift\lib\cpp\src\thrift\transport\TPipe.cpp: In member function 'virtual uint32_t apache::thrift::transport::TWaitableNamedPipeImpl::read(uint8_t*, uint32_t)':
   C:\projects\thrift\lib\cpp\src\thrift\transport\TPipe.cpp:157:3: error: 'checkReadBytesAvailable' was not declared in this scope
     157 |   checkReadBytesAvailable(len);
         |   ^~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated due to -fmax-errors=1.
   mingw32-make[2]: *** [lib\cpp\CMakeFiles\thrift.dir\build.make:483: lib/cpp/CMakeFiles/thrift.dir/src/thrift/transport/TPipe.cpp.obj] Error 1
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org