Skip to content
New issue

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

feat: also handle the timeout in async cancel_safe() with libpq < 17 #798

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlax
Copy link
Contributor

@dlax dlax commented Apr 19, 2024

See comments from #780 (comment)

@dlax dlax force-pushed the cancel-safe-legacy-timeout branch from 4feee87 to f7f58d2 Compare April 19, 2024 06:58
@dlax
Copy link
Contributor Author

dlax commented Apr 19, 2024

Following aforementioned comments in #780, it seems to me that the cancel_safe(timeout=...) works even with legacy PQcancel but the existing test does not make it possible to check that. For example, I can make it work with the following script:

# file: t.py
import psycopg
import asyncio
import os

print(f"{psycopg.pq.version()=}")


async def main() -> None:
    dsn = os.environ["PSYCOPG_TEST_DSN"]  # host=localhost
    async with await psycopg.AsyncConnection.connect(dsn) as conn:
        print("connected")
        t = asyncio.create_task(conn.execute("select pg_sleep(1)"))
        await conn.cancel_safe(timeout=0.1)
        await t


if __name__ == "__main__":
    asyncio.run(main())

and setting sudo tc qdisc add dev lo root netem delay 150ms (to be reset with sudo tc qdisc del dev lo root after tests):

$ python t.py
psycopg.pq.version()=160002
connected
Traceback (most recent call last):
  File "/usr/lib/python3.11/asyncio/tasks.py", line 490, in wait_for
    return fut.result()
           ^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/threads.py", line 25, in to_thread
    return await loop.run_in_executor(None, func_call)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
asyncio.exceptions.CancelledError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/denis/src/psycopg3/psycopg/build/__editable__.psycopg-3.2.0.dev1-py3-none-any/psycopg/connection_async.py", line 308, in cancel_safe
    await asyncio.wait_for(to_thread(self.cancel), timeout)
  File "/usr/lib/python3.11/asyncio/tasks.py", line 492, in wait_for
    raise exceptions.TimeoutError() from exc
TimeoutError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/denis/src/psycopg3/t.py", line 18, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/denis/src/psycopg3/t.py", line 13, in main
    await conn.cancel_safe(timeout=0.1)
  File "/home/denis/src/psycopg3/psycopg/build/__editable__.psycopg-3.2.0.dev1-py3-none-any/psycopg/connection_async.py", line 310, in cancel_safe
    raise e.CancellationTimeout("cancellation timeout expired") from exc
psycopg.errors.CancellationTimeout: cancellation timeout expired

@dlax
Copy link
Contributor Author

dlax commented Apr 19, 2024

So I guess that we could adjust the test to let the proxy delay requests rather than blocking them with deaf_listen(); but I'm not sure this is possible with pproxy.

@dvarrazzo
Copy link
Member

dvarrazzo commented Apr 19, 2024

the existing test does not make it possible to check that

In the changeset where I tried this myself I changed the test this way:

diff --git a/tests/test_connection_async.py b/tests/test_connection_async.py
index f5c355b2..c53c8e5d 100644
--- a/tests/test_connection_async.py
+++ b/tests/test_connection_async.py
@@ -850,8 +850,10 @@ async def test_cancel_safe_error(aconn_cls, proxy, caplog):
 
 @pytest.mark.slow
 @pytest.mark.timing
-@pytest.mark.libpq(">= 17")
 async def test_cancel_safe_timeout(aconn_cls, proxy):
+    if not is_async(__name__) and not psycopg.capabilities.has_cancel_safe():
+        pytest.skip("timeout not supported with legacy cancel in sync mode")
+
     proxy.start()
     async with await aconn_cls.connect(proxy.client_dsn) as aconn:
         proxy.stop()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants