Jens-G commented on PR #3691: URL: https://github.com/apache/thrift/pull/3691#issuecomment-5157739313
### Code review No blocking issues. Depth accounting is symmetric between the pure-Ruby and the native path (both admit exactly 64 struct levels and raise on the 65th), containers pass the budget through unchanged on every path, and the guards added by THRIFT-6025 (negative/oversized container sizes), THRIFT-6104 (`Set` subclasses), THRIFT-6124 (reset of reused deserialization targets) and THRIFT-6013 (`BaseProtocol#skip`'s independent bound) all survive the refactor. Suggestions, none blocking: 1. The new depth check runs *before* the reused-target reset that THRIFT-6124 (`d1d7280c3`) made the unconditional first action of `read`. `reused_obj.read(prot, 0)` raises `DEPTH_LIMIT` with the previous deserialization's field values still in place, which breaks the invariant `serializer_spec.rb`'s "does not retain previous struct state when reading fails" encodes — that test exercises `EOFError`, a failure mode that existed when THRIFT-6124 landed; `DEPTH_LIMIT` is a new one. Not reachable from a wire payload (`Deserializer#deserialize` always uses the default budget, and recursive descent always targets a freshly allocated object), but `read(protocol, depth)` is public API as of this PR. Moving the raise below the reset restores the invariant. https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/struct.rb#L84-L88 Same ordering in `Union#read`: https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/union.rb#L58-L62 and in the extension, where `parse_recursive_args` raises before `rb_thrift_struct_read_recursive`'s `rb_check_frozen` / `reset_struct_field` block (L664-L667) is entered: https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/ext/struct.c#L711-L716 2. Minor: the comment "Structs consume this call-local budget" sits on `static int recursion_limit`, which is file-scope and assigned once in `Init_struct`. It describes the `remaining_depth` parameter, not this variable. https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/ext/struct.c#L36-L40 On the `Init_struct` thread: your reading is right. `Init_thrift_native` resolves `Thrift`, `Thrift::Types` and `Thrift::ProtocolException::DEPTH_LIMIT` unconditionally before it reaches `Init_struct`, so a standalone `require "thrift_native"` already fails earlier, and a C-side literal would only add a second copy of the default — the drift I flagged as issue 3 on the Python equivalent, [#3592](https://github.com/apache/thrift/pull/3592#issuecomment-4674593282). Copilot's suppressed note about `write_type`'s 2→3 arity is worth a decision either way. If you keep the unconditional third argument, one sentence in the README migration note would help: omitting `remaining_depth` does not inherit the caller's budget, it restarts it at 64. https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/protocol/base_protocol.rb#L292-L299 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
