Jens-G commented on PR #3592: URL: https://github.com/apache/thrift/pull/3592#issuecomment-4674593282
### Code review Found 5 issues: 1. `increment_recursion_depth()` is placed before the `try:` block in the generated code. When the depth limit is exceeded, it raises `TProtocolException` before control enters `try:`, so the `finally: decrement_recursion_depth()` never runs. On a reused protocol object, each caught limit violation permanently increments `_recursion_depth` by 1, shrinking the effective limit. The C extension handles this correctly by decrementing before raising inside `checkDepthLimit()`. https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/compiler/cpp/src/thrift/generate/t_py_generator.cc#L1121-L1125 Fix: move the increment inside the `try:` block, or mirror the C extension by decrementing before raising in `increment_recursion_depth()` itself. 2. `setUp()` sets `self.has_fastbinary` but no test method ever checks it. When fastbinary is not built, `TBinaryProtocolAccelerated` silently falls back to pure Python, so the three accelerated tests pass while exercising only the Python path rather than the C extension. https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/lib/py/test/test_recursion_depth.py#L114-L138 3. There are three independent hardcoded `64` constants for the recursion depth limit: `DEFAULT_RECURSION_DEPTH` in TProtocol.py, `kDefaultRecursionDepth` in protocol.h, and `LIMIT` in the test file. These are not linked — changing one leaves the others stale. https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/lib/py/src/protocol/TProtocol.py#L45-L47 https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/lib/py/src/ext/protocol.h#L57-L59 4. `RecursionDepthCompactTest` and `RecursionDepthJSONTest` have no `test_read_over_limit` method. The write path is tested but not the read path for these two protocols. https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/lib/py/test/test_recursion_depth.py#L141-L174 5. `TApplicationException.read()` and `.write()` in Thrift.py call `readStructBegin`/`writeStructBegin` directly without `increment_recursion_depth()`/`decrement_recursion_depth()` guards, unlike all generated structs. `TApplicationException` has only flat primitive fields so there is no actual recursion risk, but the inconsistency means depth tracking is not accurate across a full message exchange. https://github.com/apache/thrift/blob/3cc3faa19bbe18f2bd654db74c427d3657b466b8/lib/py/src/Thrift.py#L138-L171 🤖 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]
