Andrew Hilger

This commit **fixes a flaky test**, `test_queue_timeout`, in the **Thrift Python client/server test suite** by addressing an issue where `time.sleep(1)` blocked the event loop, preventing concurrent requests from queuing. The **test fix** now uses `await asyncio.sleep(1)` to yield the event loop while keeping CPU workers blocked, and dynamically sends `num_workers + 5` requests to guarantee queue saturation. This **maintenance** ensures the **queue timeout functionality** is reliably tested and validated, removing the need for the previous skip decorator.

This commit **fixes a flaky concurrency test** in the **Python Thrift server testing suite** by addressing intermittent "Load Shedding Due to Queue Timeout" errors. It resolves the issue in `test_bidi_service_str_request_concurrently` by **increasing the server's queue timeout** from 100ms to 1.0s during the test execution. This was achieved by modifying `thrift/lib/python/test/test_server.py` to add an optional `queue_timeout` parameter to the `TestServer` constructor, which then configures the underlying `ThriftServer`. The `thrift/lib/python/test/bidi_stress.py` test now passes `queue_timeout=1.0` to the `TestServer`, allowing it sufficient time to process 10,000 concurrent bidirectional stream operations. This **maintenance fix** significantly improves the reliability of the **Thrift Python test infrastructure**.

This commit **adds new test cases** to reproduce a **critical hang bug** affecting the **asynchronous stream cancellation** mechanism in the Python Thrift library. Specifically, these tests demonstrate that **sink cancellation does not propagate promptly** when a server handler returns early while the client's async generator is blocked, leading to indefinite hangs. The new `test_bidi_sink_cancel_propagates_during_blocked_anext` in `test/bidi.py` and `test_sink_cancel_propagates_during_blocked_anext` in `test/sink.py` are currently marked `expectedFailure`, highlighting the issue in both **bidirectional streams** and **plain sink** paths. This **bug reproduction** work is crucial for addressing the robustness and resource management of **asynchronous Thrift services** that utilize these streaming patterns, alongside a minor **build dependency fix** for the bidi test.

This commit **refactors** the **Thrift Python library's testing infrastructure** by moving a **bidirectional streaming stress test** into its own dedicated file, `thrift/lib/python/test/bidi_stress.py`, and a separate build target. This **maintenance** change, which includes the `echo` and `_assert_stream` methods, significantly improves the **developer experience** and **test organization** for local execution of bidi tests. The stress test was originally designed to trigger a memory lifetime bug, and its isolation now provides a **quality of life improvement** for developers.

This commit **enhances the robustness of stream cancellation** within the **Thrift Python library**, specifically addressing **server-side async generator cleanup**. It ensures that resources are properly released and `finally` blocks are executed even when a client disconnects mid-stream and an error occurs within the generator's `finally` block. This **bug fix** prevents potential resource leaks or hangs in complex streaming scenarios by adding new test cases, `test_stream_cancel_cleanup_despite_finally_error` and `test_stream_cancel_cleanup_despite_async_finally_error`, to `thrift/lib/python/test/stream.py` to verify this critical error handling behavior.

This commit **reduces log verbosity** within the **Thrift Python streaming library** by suppressing unnecessary traceback prints. It modifies the `runGenerator` function in `thrift/lib/python/streaming/py_promise.pyx` to prevent printing a traceback to stderr when a server-side coroutine is cancelled. This **maintenance improvement** addresses a common scenario where client disconnections are expected, preventing excessive and noisy logging that can obscure more critical issues. The change improves **server observability** by ensuring logs are cleaner and more focused, while still preserving tracebacks for client-side cancellations.

This commit **fixes a critical cancellation propagation bug** within the **Python Thrift compiler's generated service code** for **bidirectional (bidi) streams**. It replaces the custom `_handle_stream` async generator with `CloseableGenerator` in the `services.mustache` template, specifically for bidi stream wrappers. This ensures that when `aclose()` is invoked on a bidi stream wrapper while `__anext__()` is blocked, the cancellation signal is correctly propagated to the underlying handler's generator. The change significantly improves the **robustness and reliability of stream termination** in generated Python Thrift services.

This commit introduces a **new capability** to the **Thrift Python streaming library** by adding `CloseableGenerator`, a class-based async iterator wrapper. This wrapper resolves a critical issue where Python's native async generators would raise a `RuntimeError` if `aclose()` was called during an in-flight `__anext__()`, preventing proper cancellation in **server-side streaming**. `CloseableGenerator` enables robust cancellation by wrapping `__anext__()` calls in an `asyncio.Task`, allowing `aclose()` to safely propagate cancellation to the underlying generator. It also incorporates `UserExceptionMeta` for describing stream exceptions and **refactors** existing stream wrapper functionality, significantly improving the reliability and error handling of **async stream processing**.

This commit introduces `CloseableGenerator` as the new wrapper for **server-side streams** within **Thrift-Python generated services**, replacing the previous async generator implementation. This change **fixes a critical bug** where **stream cancellation failed to propagate** when `__anext__()` was in-flight, as `RuntimeError` was being swallowed. By wrapping each `__anext__()` call in an `asyncio.Task`, `CloseableGenerator` enables **external cancellation**, ensuring that **asynchronous Thrift services** can properly terminate streams. This significantly improves the reliability and resource management of **streaming RPCs**, as evidenced by the removal of `unittest.expectedFailure` from `test_stream_cancel_propagates_during_blocked_anext`.

This commit introduces a **new test case** in `thrift/lib/python/test/bidi.py` to demonstrate a **critical bug** in **bidirectional stream cancellation**. The test specifically highlights that when a client disconnects while the server-side bidi stream handler's `__anext__()` is blocked, cancellation does not propagate to the handler's generator. Marked as `unittest.expectedFailure`, this test documents an existing issue within the **Thrift Python library's bidirectional streaming functionality**, where the current async generator pattern prevents `aclose()` from effectively cancelling a blocked `__anext__()`. This addition helps track the bug and is analogous to an existing stream cancellation test.

This commit **refactors** the **Thrift Python stream cancellation regression test** located in `thrift/lib/python/test/stream.py`. The change replaces `threading.Event` and `run_in_executor` with native `asyncio` equivalents such as `asyncio.Event` and `asyncio.wait_for`, making the test fully asynchronous and removing unnecessary overhead. This **cleanup** improves the test's efficiency, readability, and adherence to `asyncio` best practices. Additionally, the docstring for the test was trimmed to focus on essential contract descriptions.

This commit **adds a new test case** to `thrift/lib/python/test/stream.py` that **demonstrates a critical bug** in the **Thrift Python library's async-generator stream wrapper**. The test specifically exposes a scenario where server-side `aclose()` fails to propagate cancellation when `__anext__()` is blocked, preventing proper resource cleanup after a client disconnects. This **test addition** simulates a client disconnecting while the server's async generator is blocked, asserting that the generator is correctly cancelled. Currently marked as `unittest.expectedFailure`, this **test addition** serves as a clear reproduction for an upcoming fix to ensure robust **stream cancellation propagation** within the Python Thrift server.

This commit introduces a **robustness improvement** to the **Thrift Python service generation** by preventing server crashes. It **fixes a bug** in the `_handle_sink` method within the generated Python services, which previously caused an assertion failure and server termination when an empty or garbled `IOBuf` sink element was received, typically due to client cancellation or termination. The updated `thrift/compiler/generate/templates/python/services/services.mustache` template now ensures that such empty sink elements are gracefully handled as a normal sink closure, significantly **improving the stability of Thrift Python servers** under adverse network conditions.

This commit **removes the obsolete `thrift.lib.py3_to_python.serializer` module**, which was an intermediate solution for dispatching serialization calls between `thrift.py3` and `thrift.python` types. Since `thrift.py3.serializer` is now forward-compatible, this **cleanup and refactoring** effort migrates all **10 customer Python files** that depended on the old module to directly use `thrift.py3.serializer` or `thrift.python.serializer`. This simplifies the **Thrift serialization infrastructure** by consolidating functionality and updating corresponding BUCK file dependencies. The change also involves replacing `Protocol as PythonPy3Protocol` aliases and `GENERIC_THRIFT_STRUCT` usage, culminating in the **deletion of the entire `thrift/lib/py3_to_python/` directory**.

This commit **adds a new test case** to the **Thrift Python3 runtime** to validate and document an **inconsistent behavior of `isset`** during enum deserialization. It specifically targets scenarios where an unset, unqualified enum field, using a newly defined enum like `NoSaneDefault` that lacks a zero-valued member, results in `BadEnum` being produced, yet `isset` incorrectly reports `True`. This **test addition** exposes a critical nuance in the **Thrift deserialization logic**, highlighting that `isset` True does not always imply valid data and revealing **protocol-specific discrepancies** between JSON and BINARY/COMPACT. This work is crucial for understanding existing behavior and informing future decisions regarding `isset` usage or removal in customer code.

This commit **adds new tests** to `thrift/lib/py3/test/auto_migrate/serializer.py` to verify the behavior of `isset` with **terse fields** and **unions** in Python 3 Thrift. Specifically, it asserts that `isset` correctly excludes terse fields and reference fields, and that for unions, `isset` consistently returns `False`. These **test additions** clarify the expected `isset` behavior, which indicates if a field's value differs from its non-default state in `thrift-python`. The new assertions are integrated into the `test_terse_struct_serialization` function, improving the robustness of the **Thrift Python 3 serialization** module.

This commit **refines the assertions** for `isset` behavior within the **Thrift Python 3 test suite**, specifically in `thrift/lib/py3/test/auto_migrate/structs.py`. Based on new insights into `isset` semantics, particularly concerning `ref` fields (which are excluded from Py3) and the complex interaction with `optional` fields having default values, the `assert_isset` helper function has been updated. This **test improvement** provides more specific checks for different field types (ref, optional, unqualified) and deserialization states. The change **enhances the accuracy and robustness of `isset` validation** in Python 3 Thrift, preventing potential misinterpretations of field presence.

This commit delivers a **bug fix** for **type correctness** within the **Thrift Python library's metadata generation**. It resolves an issue where `gen_metadata` overloads were ignored due to an unknown `ServiceInterface` type, hindering static analysis. The fix involves updating the type hint for the `obj_or_cls` parameter in `thrift/lib/python/metadata.py` to correctly include `Client` and `Type[Client]`. Furthermore, it corrects the import path for `ServiceInterface` in the `thrift/lib/python/metadata.pyi` type stub, which also allows for the removal of related `pyre-fixme` comments. This ensures that type checkers can accurately validate **Thrift service interfaces and clients**, improving code reliability.

This commit introduces a **new Thrift annotation**, `python.EnableUnsafeIssetInspection`, within the `thrift/annotation/python.thrift` file. This **preparatory feature** allows specific Thrift structs to explicitly opt-in to using the `isset` API, which is slated for default disablement in Python. The change lays the groundwork for a future **deprecation** of the `isset` API, ensuring that only annotated structs will retain access to this functionality in generated **Python code**. This provides a controlled mechanism for managing the transition away from `isset` usage, impacting **Python Thrift client behavior** for field inspection.

This commit provides a **bug fix** for a circular import issue in the **Python Thrift compiler's metadata generation**. It resolves an `ImportError` that could occur due to a dependency loop between `apache.thrift.metadata.thrift_types` and `apache.thrift.metadata.thrift_enums`. The fix involves modifying compiler templates (`thrift_enums.py.mustache`, `metadata_return_type.mustache`, `metadata_thrift_type.mustache`) to use **inline imports** and conditional type definitions. This prevents import-order dependent failures in generated Python code, specifically impacting `thrift/lib/thrift/metadata.thrift` by breaking the circular dependency.