[PR] THRIFT-6182: Keep the configured maximum frame size when wrapping (Java) [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G opened a new pull request, #3785:
URL: https://github.com/apache/thrift/pull/3785

   Wrapping a transport in `TFramedTransport` or `TFastFramedTransport` overwrote the maximum frame size it was configured with, even when no maximum had been asked for:
   
   ```java
   public TFramedTransport(TTransport transport) throws TTransportException {
     this(transport, TConfiguration.DEFAULT_MAX_FRAME_SIZE);
   }
   
   public TFramedTransport(TTransport transport, int maxLength) throws TTransportException {
     TConfiguration _configuration = ... transport.getConfiguration();
     _configuration.setMaxFrameSize(maxLength);
   ```
   
   So
   
   ```java
   socket.getConfiguration().setMaxFrameSize(1024);
   TTransport framed = new TFramedTransport(socket);
   ```
   
   left the socket configured for `16384000`.
   
   Two things went wrong at once. A maximum the operator set was replaced by the library default; and the object written into belongs to the transport underneath, so anything else sharing that `TConfiguration` changed with it. A layered transport has no configuration of its own to write into — `TLayeredTransport.getConfiguration()` delegates to the inner transport by design.
   
   `TFastFramedTransport(underlying)` and `TFastFramedTransport(underlying, initialBufferCapacity)` did the same, as did `TFramedTransport.Factory()` and `TFastFramedTransport.Factory()` / `Factory(initialCapacity)` — so a server configured with `new TFramedTransport.Factory()` raised the limit on **every connection it accepted**.
   
   ### The change
   
   A constructor or factory that was not given a maximum now passes back the one the transport already carries, so wrapping changes nothing. Given one explicitly, the behaviour is unchanged: it is applied, as the argument's javadoc promises.
   
   ### The rest of the tree
   
   Go solved this for its own deprecated constructors, and the reason is written into its `TConfiguration`:
   
   > `noPropagation` — *"Used internally by deprecated constructors, to avoid overriding underlying TTransport/TProtocol's cfg by accidental propagations."*
   
   C++ (`TFramedTransport` copies `configuration_->getMaxFrameSize()` into its own `maxFrameSize_`), netstd, Delphi and Haxe all read the configured maximum and never write it. Java was the only binding where wrapping wrote it.
   
   ### Tests
   
   Four in `TestTFramedTransport`, inherited by `TestTFastFramedTransport`, so both classes are covered:
   
   - wrapping keeps a lowered maximum,
   - the factory keeps it,
   - an explicit maximum is still applied,
   - and — the point of the other three — a frame declaring 2048 bytes is still refused by a transport configured for 1024 after being wrapped.
   
   Three of the four fail on each class beforehand (`expected: <1024> but was: <16384000>`, and *"Expected TTransportException to be thrown, but nothing was thrown"*). The explicit-maximum one is a regression guard and passes either way.
   
   Full `gradle test spotlessCheck`: BUILD SUCCESSFUL.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

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

Re: [PR] THRIFT-6182: Keep the configured maximum frame size when wrapping (Java) [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G merged PR #3785:
URL: https://github.com/apache/thrift/pull/3785


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

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