THRIFT-6183: Use the library-wide default frame size in TNonblockingServer - #3793
Merged
Conversation
…erver
TNonblockingServer caps the frame it will accept at its own MAX_FRAME_SIZE,
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, described in its own header
as "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 nowhere in TNonblockingServer.h or .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.
So point MAX_FRAME_SIZE at TConfiguration::DEFAULT_MAX_FRAME_SIZE. The
constant is private, so this is not an API change, and setMaxFrameSize()
still overrides it.
This lowers a shipped default. A deployment that accepts frames between
16,384,000 and 268,435,456 bytes on TNonblockingServer today and does not
call setMaxFrameSize() will start closing those connections, so it needs a
release note.
One test. It fails before the change on 268435456 != 16384000, and pins that
the setter still wins so the escape hatch cannot quietly go away.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TNonblockingServercaps the frame it will accept from a connection at its ownMAX_FRAME_SIZE, which has been256 * 1024 * 1024since THRIFT-1337 landed it in 2011 — years beforeTConfigurationexisted. The library-wide default isTConfiguration::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:
TConfigurationappears zero times inTNonblockingServer.handTNonblockingServer.cpp, so the server neither reads a configured value nor inherits the default. It starts from its own constant, and onlysetMaxFrameSize()moves it. That constant is what sizes the read buffer, sinceTConnection::transition()resets the buffer to the length the peer declared before any payload byte has arrived.Java's
AbstractNonblockingServerhas no such gap: its per-frame limit comes fromtrans_.getMaxFrameSize(), that is fromTConfiguration, and it keeps a separate aggregate read-buffer budget on top of it.The change
MAX_FRAME_SIZEnow points atTConfiguration::DEFAULT_MAX_FRAME_SIZE. The constant isprivate, so this is not an API change, andsetMaxFrameSize()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 callsetMaxFrameSize(), will start closing those connections. Nothing in the tree relies on the old value — there is nosetMaxFrameSize()caller outside the new test, and the largest payload anywhere near this path isStressTestNonBlocking'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:and also pins that
setMaxFrameSize()still wins, so the escape hatch cannot quietly go away.cteston this branch: 32 of 35. The three failures —UnitTests,TInterruptTest,TServerIntegrationTest(SEGFAULT) — are allconnect() failed: Connection refusedand reproduce identically on pristinemasterin the same container, so they are environmental and pre-existing.Not in this PR
TNonblockingServerstill does not consultTConfigurationat all. Wiring it up is a larger design change and belongs in its own ticket.lib/cpp/src/thrift/transport/TBufferTransports.h:349still carriesTFramedTransport::DEFAULT_MAX_FRAME_SIZE = 256 * 1024 * 1024, which nothing in the tree reads any more — all three constructors takeconfiguration_->getMaxFrameSize(). That one ispublic, so correcting or removing it is an API question, unlike this change.🤖 Generated with Claude Code