From 13c570d639503218924e9547f3b0043af5787883 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Thu, 23 Mar 2023 17:36:07 +0100 Subject: [PATCH 1/4] session_db: reconnect if needed If the connection to the database fails when retrying a session operation, we end up with no cursore, which makes subsequent session operations fail. We fix this by ensuring we have cursor before any operations. --- session_db/pg_session_store.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/session_db/pg_session_store.py b/session_db/pg_session_store.py index 849e431de..5dc02fe0d 100644 --- a/session_db/pg_session_store.py +++ b/session_db/pg_session_store.py @@ -45,6 +45,7 @@ def with_cursor(func): while True: tries += 1 try: + self._ensure_connection() return func(self, *args, **kwargs) except (psycopg2.InterfaceError, psycopg2.OperationalError) as e: _logger.info("Session in DB connection Retry %s/5" % tries) @@ -67,6 +68,11 @@ class PGSessionStore(sessions.SessionStore): if self._cr is not None: self._cr.close() + @with_lock + def _ensure_connection(self): + if self._cr is None: + self._open_connection() + @with_lock def _open_connection(self): # return cursor to the pool From 0af0424b4f0b2b4e4c2f8e527dc22cbb602d74fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Thu, 23 Mar 2023 20:09:59 +0100 Subject: [PATCH 2/4] session_db: add a few tests --- session_db/tests/__init__.py | 1 + session_db/tests/test_pg_session_store.py | 70 +++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 session_db/tests/__init__.py create mode 100644 session_db/tests/test_pg_session_store.py diff --git a/session_db/tests/__init__.py b/session_db/tests/__init__.py new file mode 100644 index 000000000..22a56c889 --- /dev/null +++ b/session_db/tests/__init__.py @@ -0,0 +1 @@ +from . import test_pg_session_store diff --git a/session_db/tests/test_pg_session_store.py b/session_db/tests/test_pg_session_store.py new file mode 100644 index 000000000..8e57ce53b --- /dev/null +++ b/session_db/tests/test_pg_session_store.py @@ -0,0 +1,70 @@ +from unittest import mock + +import psycopg2 + +from odoo import http +from odoo.sql_db import connection_info_for +from odoo.tests.common import TransactionCase +from odoo.tools import config + +from odoo.addons.session_db.pg_session_store import PGSessionStore + + +def _make_postgres_uri( + login=None, password=None, host=None, port=None, database=None, **kwargs +): + uri = ["postgres://"] + if login: + uri.append(login) + if password: + uri.append(f":{password}") + uri.append("@") + if host: + uri.append(host) + if port: + uri.append(f":{port}") + uri.append("/") + if database: + uri.append(database) + return "".join(uri) + + +class TestPGSessionStore(TransactionCase): + def setUp(self): + super().setUp() + _, connection_info = connection_info_for(config["db_name"]) + self.session_store = PGSessionStore( + _make_postgres_uri(**connection_info), session_class=http.Session + ) + + def test_session_crud(self): + session = self.session_store.new() + session["test"] = "test" + self.session_store.save(session) + assert session.sid is not None + assert self.session_store.get(session.sid)["test"] == "test" + self.session_store.delete(session) + assert self.session_store.get(session.sid).get("test") is None + + def test_retry(self): + """Test that session operations are retried before failing""" + with mock.patch("odoo.sql_db.Cursor.execute") as mock_execute: + mock_execute.side_effect = psycopg2.OperationalError() + with self.assertRaises(psycopg2.OperationalError): + self.session_store.get("abc") + assert mock_execute.call_count == 5 + # when the error is resolved, it works again + self.session_store.get("abc") + + def test_retry_connect_fail(self): + with mock.patch("odoo.sql_db.Cursor.execute") as mock_execute, mock.patch( + "odoo.sql_db.db_connect" + ) as mock_db_connect: + mock_execute.side_effect = psycopg2.OperationalError() + mock_db_connect.side_effect = RuntimeError("connection failed") + # get fails, and a RuntimeError is raised when trying to reconnect + with self.assertRaises(RuntimeError): + self.session_store.get("abc") + assert mock_execute.call_count == 1 + # when the error is resolved, it works again + self.session_store.get("abc") From 2344802039fe9de267c02a1de60a5533b14b3527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Fri, 24 Mar 2023 09:38:44 +0100 Subject: [PATCH 3/4] session_db: refactor retry handling --- session_db/pg_session_store.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/session_db/pg_session_store.py b/session_db/pg_session_store.py index 5dc02fe0d..cf2dd607d 100644 --- a/session_db/pg_session_store.py +++ b/session_db/pg_session_store.py @@ -47,11 +47,14 @@ def with_cursor(func): try: self._ensure_connection() return func(self, *args, **kwargs) - except (psycopg2.InterfaceError, psycopg2.OperationalError) as e: - _logger.info("Session in DB connection Retry %s/5" % tries) + except (psycopg2.InterfaceError, psycopg2.OperationalError): + self._close_connection() if tries > 4: - raise e - self._open_connection() + _logger.warning( + "session_db operation try %s/5 failed, aborting", tries + ) + raise + _logger.info("session_db operation try %s/5 failed, retrying", tries) return wrapper @@ -65,8 +68,7 @@ class PGSessionStore(sessions.SessionStore): self._setup_db() def __del__(self): - if self._cr is not None: - self._cr.close() + self._close_connection() @with_lock def _ensure_connection(self): @@ -75,16 +77,20 @@ class PGSessionStore(sessions.SessionStore): @with_lock def _open_connection(self): - # return cursor to the pool + self._close_connection() + cnx = odoo.sql_db.db_connect(self._uri, allow_uri=True) + self._cr = cnx.cursor() + self._cr._cnx.autocommit = True + + @with_lock + def _close_connection(self): + """Return cursor to the pool.""" if self._cr is not None: try: self._cr.close() except Exception: # pylint: disable=except-pass pass self._cr = None - cnx = odoo.sql_db.db_connect(self._uri, allow_uri=True) - self._cr = cnx.cursor() - self._cr._cnx.autocommit = True @with_lock @with_cursor From 8148f43ca2daae533e2d77810eb64d7996b8b998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Mon, 27 Mar 2023 19:01:20 +0200 Subject: [PATCH 4/4] session_db: fix tests for v16 compatibility --- session_db/tests/test_pg_session_store.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/session_db/tests/test_pg_session_store.py b/session_db/tests/test_pg_session_store.py index 8e57ce53b..61e7500ec 100644 --- a/session_db/tests/test_pg_session_store.py +++ b/session_db/tests/test_pg_session_store.py @@ -50,8 +50,14 @@ class TestPGSessionStore(TransactionCase): """Test that session operations are retried before failing""" with mock.patch("odoo.sql_db.Cursor.execute") as mock_execute: mock_execute.side_effect = psycopg2.OperationalError() - with self.assertRaises(psycopg2.OperationalError): + try: self.session_store.get("abc") + except psycopg2.OperationalError: # pylint: disable=except-pass + pass + else: + # We don't use self.assertRaises because Odoo is overriding + # in a way that interferes with the Cursor.execute mock + raise AssertionError("expected psycopg2.OperationalError") assert mock_execute.call_count == 5 # when the error is resolved, it works again self.session_store.get("abc") @@ -63,8 +69,14 @@ class TestPGSessionStore(TransactionCase): mock_execute.side_effect = psycopg2.OperationalError() mock_db_connect.side_effect = RuntimeError("connection failed") # get fails, and a RuntimeError is raised when trying to reconnect - with self.assertRaises(RuntimeError): + try: self.session_store.get("abc") + except RuntimeError: # pylint: disable=except-pass + pass + else: + # We don't use self.assertRaises because Odoo is overriding + # in a way that interferes with the Cursor.execute mock + raise AssertionError("expected RuntimeError") assert mock_execute.call_count == 1 # when the error is resolved, it works again self.session_store.get("abc")