From 500fc32f33e08137206e66dd32ce5cb71e11ccf2 Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Mon, 30 Oct 2023 08:54:16 +0100 Subject: [PATCH 1/5] Add regression test for #390 --- .../regression/test_regression_bug390.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 test/integration/regression/test_regression_bug390.py diff --git a/test/integration/regression/test_regression_bug390.py b/test/integration/regression/test_regression_bug390.py new file mode 100644 index 00000000..e3013270 --- /dev/null +++ b/test/integration/regression/test_regression_bug390.py @@ -0,0 +1,37 @@ +from inspect import cleandoc + +pytest_plugins = "pytester" + + +def test_connection_with_block_cleans_up_properly(pytester, itde): + config = itde.db + # Because the error only occurs on process shutdown we need to run a test within a test + # (We require the result (stderr) of a terminated process triggering the failure. + pytester.makepyfile( + # fmt: off + cleandoc( + f""" + from sqlalchemy import create_engine + + def test(): + url = "exa+websocket://{{user}}:{{pw}}@{{host}}:{{port}}/{{schema}}?SSLCertificate=SSL_VERIFY_NONE" + url = url.format( + user="{config.username}", + pw="{config.password}", + host="{config.host}", + port={config.port}, + schema="{itde.itde.schemas[0]}", + ) + engine = create_engine(url) + query = "SELECT 42;" + with engine.connect() as con: + result = con.execute(query).fetchall() + """ + ), + # fmt: on + ) + r = pytester.runpytest_subprocess() + expected = "" + actual = f"{r.stderr}" + + assert actual == expected From dc3a230900ce57636047a93c638a9e951fb1e205 Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Fri, 3 Nov 2023 07:57:32 +0100 Subject: [PATCH 2/5] Improve status checks before closing underlying connection --- exasol/driver/websocket/_connection.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/exasol/driver/websocket/_connection.py b/exasol/driver/websocket/_connection.py index 20c1944b..07ee29b0 100644 --- a/exasol/driver/websocket/_connection.py +++ b/exasol/driver/websocket/_connection.py @@ -119,10 +119,12 @@ def connection(self): def close(self): """See also :py:meth: `Connection.close`""" - if not self._connection: + connection_to_close = self._connection + self._connection = None + if connection_to_close is None or connection_to_close.is_closed: return try: - self._connection.close() + connection_to_close.close() except Exception as ex: raise Error() from ex From ec91a360e17d2c91e086cc8e4a2c4a4b20a61fcd Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Fri, 3 Nov 2023 10:52:43 +0100 Subject: [PATCH 3/5] Handle __del__ more gracefully --- exasol/driver/websocket/_connection.py | 14 +++++++++++++- .../regression/test_regression_bug390.py | 8 ++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/exasol/driver/websocket/_connection.py b/exasol/driver/websocket/_connection.py index 07ee29b0..dd239a45 100644 --- a/exasol/driver/websocket/_connection.py +++ b/exasol/driver/websocket/_connection.py @@ -150,4 +150,16 @@ def cursor(self): return DefaultCursor(self) def __del__(self): - self.close() + if self._connection is None: + return + + # Currently, the only way to handle this gracefully is to invoke the`__del__` + # method of the underlying connection rather than calling an explicit `close`. + # + # For more details, see also: + # * https://github.com/exasol/sqlalchemy-exasol/issues/390 + # * https://github.com/exasol/pyexasol/issues/108 + # + # If the above tickets are resolved, it should be safe to switch back to using + # `close` instead of `__del__`. + self._connection.__del__() diff --git a/test/integration/regression/test_regression_bug390.py b/test/integration/regression/test_regression_bug390.py index e3013270..b634001b 100644 --- a/test/integration/regression/test_regression_bug390.py +++ b/test/integration/regression/test_regression_bug390.py @@ -3,14 +3,14 @@ pytest_plugins = "pytester" -def test_connection_with_block_cleans_up_properly(pytester, itde): - config = itde.db +def test_connection_with_block_cleans_up_properly(pytester, exasol_config): + config = exasol_config # Because the error only occurs on process shutdown we need to run a test within a test # (We require the result (stderr) of a terminated process triggering the failure. pytester.makepyfile( # fmt: off cleandoc( - f""" + f""" from sqlalchemy import create_engine def test(): @@ -20,7 +20,7 @@ def test(): pw="{config.password}", host="{config.host}", port={config.port}, - schema="{itde.itde.schemas[0]}", + schema="TEST", ) engine = create_engine(url) query = "SELECT 42;" From 9aaa27bddb9b16c83c3d269cbd397548a23048a6 Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Fri, 3 Nov 2023 13:07:16 +0100 Subject: [PATCH 4/5] Update test/integration/regression/test_regression_bug390.py Co-authored-by: Christoph Pirkl --- test/integration/regression/test_regression_bug390.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/regression/test_regression_bug390.py b/test/integration/regression/test_regression_bug390.py index b634001b..ad0fe804 100644 --- a/test/integration/regression/test_regression_bug390.py +++ b/test/integration/regression/test_regression_bug390.py @@ -32,6 +32,6 @@ def test(): ) r = pytester.runpytest_subprocess() expected = "" - actual = f"{r.stderr}" + actual = str(r.stderr) assert actual == expected From 0156fc9f9d6bc55c0226b0e73c3c9f96109f9aad Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Fri, 3 Nov 2023 14:03:36 +0100 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 75b4e867..cf3570e7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,6 +3,11 @@ Unreleased ========== +🐞 Fixed +--------- + +- Fixed `WebSocket connection isn't properly closed in case of process termination `_ + .. _changelog-4.6.0: 4.6.0 — 2023-07-18