Jens Geyer created THRIFT-6245:
----------------------------------
Summary: Grow the D TNonblockingServer read buffer as the payload
arrives, not on the frame header
Key: THRIFT-6245
URL: https://issues.apache.org/jira/browse/THRIFT-6245
Project: Thrift
Issue Type: Bug
Components: D - Library
Reporter: Jens Geyer
Fix For: 0.25.0
{{TNonblockingServer}}'s {{Connection}} reserves the whole declared frame the
moment it reads the four-byte length prefix: {{transition()}} in the
{{ConnectionState.READ_FRAME_SIZE}} case reallocates the read buffer to the
frame size, rounded up to the next power of two
({{lib/d/src/thrift/server/nonblocking.d}}), before any payload byte has
arrived. A peer can therefore make the server reserve up to {{maxFrameSize}}
bytes per connection by sending only a four-byte header, with
{{DEFAULT_MAX_CONNECTIONS}} being {{int.max}}. The reservation is address space
rather than resident memory (it is a C {{realloc}}, not the GC), but it is
still an unamplified header-to-allocation step.
This is the D counterpart of THRIFT-6243, which made the same change in C++.
THRIFT-6241 lowered D's default frame size to the library-wide value; this
change bounds the allocation by what the peer actually sends:
* {{transition()}} reserves only enough to begin reading -- about a kilobyte --
rather than the declared frame.
* The buffer grows toward the frame size in the {{SocketState.RECV}} state as
bytes are actually read, by the same doubling the up-front allocation used, now
driven by what the peer sends rather than by the size it declares.
* The doubling moves into a {{growReadBuffer()}} helper that additionally stops
before overflowing {{size_t}}. The old in-line loop would have spun forever on
a 32-bit {{size_t}} with {{maxFrameSize}} raised past 2 GiB.
One consequence is worth calling out for the reviewer, because it is not a
straight translation of the C++ change. The socket read was slicing
{{readBuffer_[readBufferPos_ .. readWant_]}}, which only ever stayed inside the
allocation *because* the whole frame had been reserved first. Once the buffer
can be smaller than the frame that slice runs past the end of it, and D does
not bounds-check a slice taken from a raw pointer even in a debug build. The
read is therefore capped at {{min(readBufferSize_, readWant_)}}. The buffer can
also be larger than the frame -- doubling may overshoot, or it may have been
kept from an earlier, larger request -- and bytes past {{readWant_}} belong to
the next frame.
A frame that fully arrives ends at the same buffer size as before (the next
power of two at or above the frame size), so there is no change for legitimate
traffic in the steady state, and none at all for frames of a kilobyte or less.
A header with no payload reserves next to nothing.
The initial reservation is deliberately {{DEFAULT_IDLE_READ_BUFFER_LIMIT}}: a
connection that only ever carries small frames settles at exactly the size
{{checkIdleBufferLimit()}} lets an idle connection keep, so it is not freed and
regrown between requests. It is a fixed constant on purpose -- deriving it from
the configurable idle limit would mean that raising that limit put every
connection back to reserving on the header.
Tradeoff, for the reviewer: a large frame that does arrive now pays the
amortized doubling copy that any grow-as-you-go buffer pays, on the order of
twice the frame size in copying over the life of the connection, against a
single allocation before. This is the cost of not trusting the declared size.
h2. Tests
Two cases in {{nonblocking.d}}, both written before the change and both proven
two-state:
* A header declaring 64 MiB followed by no payload leaves the connection's read
buffer at a kilobyte. Before the change it was 67108864 bytes. The test closes
the client so the connection goes back on the idle stack, where its buffer can
be read without racing the I/O thread, with {{idleReadBufferLimit}} set to 0 so
disposal does not free it first.
* Two 2 MiB requests over one connection, each filled with a position-dependent
pattern and checked against a hash the server computes and returns. Each frame
is far larger than the initial reservation, so it is assembled over many
libevent callbacks while the buffer doubles, and it has to round-trip byte for
byte.
Neutralising either half of the change fails: restoring the full-frame
reservation puts the first case back to 67108864 bytes, and restoring the
uncapped slice aborts the second with {{free(): corrupted unsorted chunks}}.
The existing frame-size unittest's server harness is hoisted to
{{version(unittest)}} module scope, unchanged, so both tests can use it.
Verified with dmd 2.087.0, debug and release, three runs each, plus a clean
{{-w -wi}} library build of {{lib/d/src}} in both modes.
_Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
