rusackas commented on code in PR #44908:
URL: https://github.com/apache/superset/pull/44908#discussion_r4171921283
##########
tests/testcontainers/db_engine_specs/test_oracle.py:
##########
@@ -93,3 +97,85 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
for col in by_name.values():
spec = OracleEngineSpec.get_column_spec(str(col["type"]))
assert spec is not None
+
+
+def test_cancel_query_stops_a_running_statement(engine: Engine) -> None:
+ """
+ apache/superset#44704 added OracleEngineSpec.get_cancel_query_id/
+ cancel_query, verified only by hand against a real Oracle instance
+ during development (per that PR's own testing notes) -- never pinned
+ down as a repeatable test. This exercises the real pair against a real
+ session: identify the session about to run a long statement on one
+ connection, cancel it from a second connection mid-flight (mirroring
+ SQL Lab's own "new cursor to the db of the query" cancel path), and
+ confirm the first connection's statement actually fails with ORA-01013
+ well before it would complete on its own -- not just that
+ ``cancel_query`` returned ``True`` without effect.
+
+ The container's default user is ``system`` (DBA), which holds the
+ ``ALTER SYSTEM`` privilege ``cancel_query`` needs; see
+ apache/superset#44704's description for the ``False``/ORA-01031 path
+ when that privilege is absent, which this does not exercise.
+ """
+ query_conn = engine.raw_connection()
+ cancel_conn = engine.raw_connection()
+ try:
+ query_cursor = query_conn.cursor()
+ # get_cancel_query_id runs on the same cursor that is about to
+ # execute the query, before it does -- mirrors SQL Lab's own order.
+ cancel_query_id = OracleEngineSpec.get_cancel_query_id(query_cursor,
Query())
+ assert cancel_query_id is not None
+
+ outcome: dict[str, Any] = {}
+
+ def run_slow_query() -> None:
+ try:
+ # A tight PL/SQL loop: CPU-bound with O(1) memory, needs no
+ # privileges beyond CREATE SESSION, and (measured directly
+ # against this same image) ~8s at 500M iterations -- plenty
+ # of margin over the 1.5s delay below. Two things this is
+ # NOT, both tried first:
+ # - `CONNECT BY LEVEL <= n`: materializes the whole
+ # hierarchy and hits ORA-30009 "not enough memory" long
+ # before a count this large finishes -- that failure has
+ # nothing to do with cancellation, so the test would pass
+ # for the wrong reason.
+ # - a loop with no loop-carried side effect (`NULL;` as the
+ # body): optimized away entirely regardless of the
+ # iteration count, so the statement returns instantly and
+ # the cancel never has anything to catch. The `cnt`
+ # assignment here is load-bearing, not cosmetic.
+ query_cursor.execute(
+ "DECLARE cnt NUMBER := 0; BEGIN "
+ "FOR i IN 1..500000000 LOOP cnt := cnt + 1; END LOOP; "
+ "END;"
+ )
+ outcome["completed"] = True
+ except Exception as ex: # noqa: BLE001 # pylint:
disable=broad-except
+ outcome["error"] = ex
+
+ thread = threading.Thread(target=run_slow_query)
+ start = time.monotonic()
+ thread.start()
+ time.sleep(1.5) # let the statement actually start executing
Review Comment:
Fair point, the fixed sleep only guesses that the statement started. Swapped
it for a poll on `v$session` until the session reports ACTIVE (bails out if the
worker dies first or after 30s), so the cancel only fires once the statement is
actually running.
##########
tests/testcontainers/db_engine_specs/test_oracle.py:
##########
@@ -93,3 +97,85 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
for col in by_name.values():
spec = OracleEngineSpec.get_column_spec(str(col["type"]))
assert spec is not None
+
+
+def test_cancel_query_stops_a_running_statement(engine: Engine) -> None:
+ """
+ apache/superset#44704 added OracleEngineSpec.get_cancel_query_id/
+ cancel_query, verified only by hand against a real Oracle instance
+ during development (per that PR's own testing notes) -- never pinned
+ down as a repeatable test. This exercises the real pair against a real
+ session: identify the session about to run a long statement on one
+ connection, cancel it from a second connection mid-flight (mirroring
+ SQL Lab's own "new cursor to the db of the query" cancel path), and
+ confirm the first connection's statement actually fails with ORA-01013
+ well before it would complete on its own -- not just that
+ ``cancel_query`` returned ``True`` without effect.
+
+ The container's default user is ``system`` (DBA), which holds the
+ ``ALTER SYSTEM`` privilege ``cancel_query`` needs; see
+ apache/superset#44704's description for the ``False``/ORA-01031 path
+ when that privilege is absent, which this does not exercise.
+ """
+ query_conn = engine.raw_connection()
+ cancel_conn = engine.raw_connection()
+ try:
+ query_cursor = query_conn.cursor()
+ # get_cancel_query_id runs on the same cursor that is about to
+ # execute the query, before it does -- mirrors SQL Lab's own order.
+ cancel_query_id = OracleEngineSpec.get_cancel_query_id(query_cursor,
Query())
+ assert cancel_query_id is not None
+
+ outcome: dict[str, Any] = {}
+
+ def run_slow_query() -> None:
+ try:
+ # A tight PL/SQL loop: CPU-bound with O(1) memory, needs no
+ # privileges beyond CREATE SESSION, and (measured directly
+ # against this same image) ~8s at 500M iterations -- plenty
+ # of margin over the 1.5s delay below. Two things this is
+ # NOT, both tried first:
+ # - `CONNECT BY LEVEL <= n`: materializes the whole
+ # hierarchy and hits ORA-30009 "not enough memory" long
+ # before a count this large finishes -- that failure has
+ # nothing to do with cancellation, so the test would pass
+ # for the wrong reason.
+ # - a loop with no loop-carried side effect (`NULL;` as the
+ # body): optimized away entirely regardless of the
+ # iteration count, so the statement returns instantly and
+ # the cancel never has anything to catch. The `cnt`
+ # assignment here is load-bearing, not cosmetic.
+ query_cursor.execute(
+ "DECLARE cnt NUMBER := 0; BEGIN "
+ "FOR i IN 1..500000000 LOOP cnt := cnt + 1; END LOOP; "
+ "END;"
+ )
+ outcome["completed"] = True
+ except Exception as ex: # noqa: BLE001 # pylint:
disable=broad-except
+ outcome["error"] = ex
+
+ thread = threading.Thread(target=run_slow_query)
+ start = time.monotonic()
+ thread.start()
+ time.sleep(1.5) # let the statement actually start executing
+
+ cancel_cursor = cancel_conn.cursor()
+ cancelled = OracleEngineSpec.cancel_query(
+ cancel_cursor, Query(), cancel_query_id
+ )
+ assert cancelled is True
+
+ thread.join(timeout=60)
+ elapsed = time.monotonic() - start
+
+ assert not thread.is_alive(), (
+ "cancel_query returned True but the statement is still running"
+ )
+ assert "error" in outcome, "the statement completed instead of being
cancelled"
+ assert "ORA-01013" in str(outcome["error"])
+ # The cancel took effect promptly, not "eventually" after the slow
+ # statement would have finished on its own regardless.
+ assert elapsed < 30
+ finally:
+ query_conn.close()
Review Comment:
Good catch. The worker thread is now a daemon, and the `finally` joins it
before closing `query_conn`, so a failed assertion no longer races the close or
hangs pytest at exit.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
