THRIFT-6161: Charge reads against the message budget in the Haxe TStreamTransport - #3757
Merged
Conversation
…eamTransport Every endpoint transport in the binding charges reads against MaxMessageSize -- TSocket, THttpClient, TFullDuplexHttpClient -- except TStreamTransport, which charged none. MaxMessageSize is specified as "a general device to be used with any transport or protocol" and is expressed as the bytes remaining to be read (doc/specs/thrift-tconfiguration.md), so on a stream-backed connection the limit had no effect at all: a message could read any amount as long as no single read exceeded it on its own. read() now charges what it read. flush() resets the allowance, at the same point TSocket does -- one without the other would leave the budget only ever shrinking, and a long-lived connection would eventually run itself out of it. There is no single convention across the bindings to be inconsistent with, but Haxe is the only one that departs from its own: cpp and c_glib charge nowhere but zlib, java and netstd charge only where the message boundary is known, delphi charges everywhere, and Haxe charges everywhere except this one endpoint. Two tests. The first reads past the limit 32 bytes at a time, so nothing but cumulative accounting can catch it; the second checks the allowance comes back. Verified on neko (full suite, including the existing stream, constants and recursion-limit tests, all of which read through TStreamTransport); the python and php targets cross-compile clean. Client: haxe 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
1 participant
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.
THRIFT-6161 — follows THRIFT-6160 (#3755), same subsystem, independent change.
TStreamTransportdoes not charge reads against the message budget, soMaxMessageSizehas no effect on a stream-backed connection however much a message reads, as long as no single read exceeds the limit on its own. Itsflush()does not restore the allowance either.Why this is a defect in Haxe specifically
There is no single convention across the bindings — there are three, and every other binding keeps to one:
TSocketyes,TStreamTransportnoHaxe is the only binding that departs from its own convention. Its four endpoint transports are
TSocket(charges, 3 sites),THttpClient(charges),TFullDuplexHttpClient(charges) andTStreamTransport(charges nothing).doc/specs/thrift-tconfiguration.mddescribesMaxMessageSizeas "a general device to be used with any transport or protocol", expressed as the bytes remaining to be read — which only means something if reads draw it down.Fix
Two coordinated changes, mirroring
TSocket:read()charges what it readflush()resets the allowance, at the same pointTSocketdoesBoth are needed. Charging without resetting would leave the budget only ever shrinking, so a long-lived connection would run itself out of it — which is exactly THRIFT-6160, fixed just before this.
Tests
Two, in
StreamTest, failing before and passing after. The first reads past the limit 32 bytes at a time, so nothing but cumulative accounting can catch it; the second checks the allowance comes back.Verified on neko — full suite, including the existing stream, constants and recursion-limit tests, all of which read through
TStreamTransportand now have charged reads. The python and php targets cross-compile clean.🤖 Generated with Claude Code