[
https://issues.apache.org/jira/browse/THRIFT-6245?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6245.
--------------------------------
Assignee: Jens Geyer
Resolution: Fixed
> 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
> Assignee: Jens Geyer
> Priority: Major
> Fix For: 0.25.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> {{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)
