Jens-G opened a new pull request, #3880: URL: https://github.com/apache/thrift/pull/3880
[THRIFT-6288](https://issues.apache.org/jira/browse/THRIFT-6288) Comment-only change: the three message-size TODOs in the Rust protocols now say what the code does today and why the more precise check is missing. ### Background The ticket quotes a TODO that is not the text in the tree. The actual comment in `check_container_size`, there since THRIFT-5871 (42d0b7177), reads: ```rust // TODO: When Rust trait specialization stabilizes, we can add more precise checks // for transports that track exact remaining bytes. For now, we use the message // size limit as a best-effort check. ``` That comment names a real obstacle. The THRIFT-5871 commit message says the same: full message-size tracking was left out because trait specialization is not stable. - `TReadTransport` is implemented for every `io::Read` by one blanket impl (`transport/mod.rs`). So no transport can supply its own remaining-bytes count without specialization or a change to the trait. - The other bindings keep that count in their transports, for example C++ `TTransport` and Java `TEndpointTransport`. What the comment did not say is what the check compares against today, which is the whole `max_message_size`. It also did not say that up-front allocation does not depend on the check. ### What changes - **`protocol/mod.rs`, `check_container_size`:** the TODO says that `min_bytes_needed` is held to the whole `max_message_size` rather than to what is left of the current message, and why. It also notes that generated code reserves list capacity through `prealloc_size`, which caps it; sets and maps are `BTreeSet`/`BTreeMap` and reserve nothing. - **`protocol/binary.rs` and `protocol/compact.rs`, `read_message_begin`:** the matching "call the message size tracking here" TODOs now say what would start there, and point to `check_container_size`. No behaviour changes, so there is no test to add. ### Verification Rust 1.85.1, as in CI, in `lib/rs`: - `cargo fmt --all -- --check`, `cargo clippy --all -- -D warnings` and `cargo clippy --all --all-features -- -D warnings` are clean. - `cargo check --no-default-features --features rustls` passes. - `cargo test` and `cargo test --all-features`: 177 of 178 pass. - The one failure is `transport::socket::tests::split_halves_must_not_clobber_each_others_timeout`. It fails the same way on unmodified master on this machine: `Some(252ms)` vs `Some(250ms)`. - The host kernel runs with `CONFIG_HZ=250`, and Linux stores socket timeouts in jiffies, so 250 ms comes back as 252 ms. The test passes in CI, so the runners' kernels keep 250 ms exact. The failure is unrelated to this change. - Merges cleanly with the open Rust PR #3854. --- The change was written with AI assistance (Claude Opus 5), then reviewed and checked as described above. 🤖 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]
