Jens-G opened a new pull request, #3793: URL: https://github.com/apache/thrift/pull/3793
`TNonblockingServer` caps the frame it will accept from a connection at its own `MAX_FRAME_SIZE`, which has been `256 * 1024 * 1024` since THRIFT-1337 landed it in 2011 — years before `TConfiguration` existed. The library-wide default is `TConfiguration::DEFAULT_MAX_FRAME_SIZE = 16384000`, whose own comment reads *"this value is used consistently across all Thrift libraries"*. The two disagree by a factor of 16: ``` TNonblockingServer::getMaxFrameSize() 268435456 TConfiguration::DEFAULT_MAX_FRAME_SIZE 16384000 ``` `TConfiguration` appears zero times in `TNonblockingServer.h` and `TNonblockingServer.cpp`, so the server neither reads a configured value nor inherits the default. It starts from its own constant, and only `setMaxFrameSize()` moves it. That constant is what sizes the read buffer, since `TConnection::transition()` resets the buffer to the length the peer declared before any payload byte has arrived. Java's `AbstractNonblockingServer` has no such gap: its per-frame limit comes from `trans_.getMaxFrameSize()`, that is from `TConfiguration`, and it keeps a separate aggregate read-buffer budget on top of it. ### The change `MAX_FRAME_SIZE` now points at `TConfiguration::DEFAULT_MAX_FRAME_SIZE`. The constant is `private`, so this is not an API change, and `setMaxFrameSize()` still overrides it. ### Compatibility — worth a reviewer's attention **This lowers a shipped default.** A deployment that today accepts frames between 16,384,000 and 268,435,456 bytes on `TNonblockingServer`, and does not call `setMaxFrameSize()`, will start closing those connections. Nothing in the tree relies on the old value — there is no `setMaxFrameSize()` caller outside the new test, and the largest payload anywhere near this path is `StressTestNonBlocking`'s 2 MB chunk size — but out-of-tree users may. Release note needed. ### Test One case added to the existing `lib/cpp/test/TNonblockingServerTest.cpp`. It fails before the change: ``` error: in "TNonblockingServerTest/default_max_frame_size_matches_configuration": check server.getMaxFrameSize() == static_cast<size_t>(TConfiguration::DEFAULT_MAX_FRAME_SIZE) has failed [268435456 != 16384000] ``` and also pins that `setMaxFrameSize()` still wins, so the escape hatch cannot quietly go away. `ctest` on this branch: 32 of 35. The three failures — `UnitTests`, `TInterruptTest`, `TServerIntegrationTest` (SEGFAULT) — are all `connect() failed: Connection refused` and reproduce identically on pristine `master` in the same container, so they are environmental and pre-existing. ### Not in this PR - `TNonblockingServer` still does not consult `TConfiguration` at all. Wiring it up is a larger design change and belongs in its own ticket. - `lib/cpp/src/thrift/transport/TBufferTransports.h:349` still carries `TFramedTransport::DEFAULT_MAX_FRAME_SIZE = 256 * 1024 * 1024`, which nothing in the tree reads any more — all three constructors take `configuration_->getMaxFrameSize()`. That one is `public`, so correcting or removing it is an API question, unlike this change. 🤖 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
