| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -342,6 +342,50 @@ def test_execute_async__large_result(self, extra_params): | |
|
|
||
| assert len(result) == x_dimension * y_dimension | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "extra_params", | ||
|
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
|
||
| [ | ||
| {}, | ||
| { | ||
| "use_sea": True, | ||
| }, | ||
| ], | ||
| ) | ||
| def test_execute_async__close_without_fetch_frees_handle(self, extra_params): | ||
|
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
|
||
| """Closing a cursor whose async command result was never fetched must free | ||
| the server-side statement handle (issue #791). Otherwise the handle leaks | ||
| until the session closes.""" | ||
| with self.cursor(extra_params) as cursor: | ||
| cursor.execute_async("SELECT 1") | ||
|
|
||
| # Capture the server-side command id before we close the cursor. | ||
| command_id = cursor.active_command_id | ||
| assert command_id is not None | ||
|
|
||
| backend = cursor.backend | ||
|
|
||
| # Sanity: the handle is live and pollable before close. | ||
| backend.get_query_state(command_id) | ||
|
|
||
| # User decides not to wait for the result and closes the cursor | ||
| # without ever calling get_async_execution_result(). | ||
| cursor.close() | ||
|
|
||
| # After close, the server-side handle must have been freed. How a | ||
| # re-poll of the saved command id surfaces that is backend-specific: | ||
| # - Thrift raises a server error on the closed handle. | ||
| # - SEA's get_query_state() does a plain GET and returns the | ||
| # status.state, so a freed statement may come back as a terminal | ||
| # CLOSED/CANCELLED state instead of raising. | ||
| # Accept either signal as proof the handle was freed. Pre-fix (leak), | ||
| # the poll instead returns a live/terminal-success state. | ||
| try: | ||
| state = backend.get_query_state(command_id) | ||
| except (RequestError, OperationalError, DatabaseError): | ||
| pass | ||
| else: | ||
| assert state in (CommandState.CLOSED, CommandState.CANCELLED) | ||
|
|
||
|
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Comment thread
Copy link
Copy Markdown
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality🔵 Low — The SEA branch of this test may be brittle. After close_command issues a DELETE on the statement, the subsequent get_query_state does a plain GET (_poll_query) and returns whatever status.state the server reports. The test only tolerates a raised error or state in (CommandState.CLOSED, CommandState.CANCELLED). If SEA responds to a GET on a just-deleted statement with any other terminal/transient state (e.g. it still echoes SUCCEEDED briefly, or a state not in that tuple), the else assert fails and the test flakes — even though the handle was correctly freed. Since get_query_state intentionally does not call _check_command_not_in_failed_or_closed_state, there's no guarantee of a CLOSED/CANCELLED signal. Consider confirming empirically what SEA returns post-delete, or broadening the accepted signal (e.g. also accept a 404-derived error) so the test isn't tied to an unverified state assumption.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
The comment targets the SEA branch of an E2E test whose correct accepted-state set depends on what a live SEA warehouse actually returns from get_query_state() after close_command issues the statement DELETE. This job has no live-warehouse connection and must not run/add e2e tests, so I cannot verify SEA's real post-delete signal here. The reviewer's two options both hinge on that empirical fact: (a) confirm empirically — impossible in this job; (b) broaden the accepted states — unsafe to do blind, because the pre-fix leak "returns a live/terminal-success state," so broadening (e.g. accepting SUCCEEDED) without knowing SEA's freed-handle behavior would let the test pass in the leak case and destroy the regression it guards. A human needs to run this against a live SEA warehouse to observe the actual post-DELETE state (or 404), then either narrow the assertion to that confirmed signal or, if SEA gives no reliable freed signal, restructure the SEA branch (e.g. skip the re-poll assertion for SEA). Flagging for human judgment rather than guessing at a broadening that could mask the leak.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| # Exclude Retry tests because they require specific setups, and LargeQueries too slow for core | ||
| # tests | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.