From 8d52482c79f26dde8d40e2e1b6aab2440120460d Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 3 Dec 2020 15:24:35 +0100 Subject: [PATCH] Make memoize behave better in concurrent environments Currently there is a chance for System view to raise sqlite3 exceptions because of table lock in PersistentDict. 1) Stop re-loading content on PersistentDict creation. 2) Retry OperationalError on all database accesses. 3) Rule out a race condition in __delitem__ Also refactor tests to remove excessive mocking. Change-Id: I82726c41578700835f3b15d316aed562d68fbf67 --- lower-constraints.txt | 1 + requirements.txt | 1 + sushy_tools/emulator/memoize.py | 35 ++-- .../tests/unit/emulator/test_memoize.py | 160 +++++++++++------- 4 files changed, 117 insertions(+), 80 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index d928e82f..351d0338 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -55,6 +55,7 @@ Sphinx==2.0.0 sphinxcontrib-websupport==1.0.1 stestr==1.0.0 stevedore==1.20.0 +tenacity==6.2.0 testrepository==0.0.20 testscenarios==0.4 testtools==2.2.0 diff --git a/requirements.txt b/requirements.txt index 740a1f9f..32a9b968 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,3 +5,4 @@ pbr!=2.1.0,>=2.0.0 # Apache-2.0 Flask>=1.0.2 # BSD requests>=2.14.2 # Apache-2.0 +tenacity>=6.2.0 # Apache-2.0 diff --git a/sushy_tools/emulator/memoize.py b/sushy_tools/emulator/memoize.py index aaadd7b4..6fe9f176 100644 --- a/sushy_tools/emulator/memoize.py +++ b/sushy_tools/emulator/memoize.py @@ -26,6 +26,8 @@ import pickle import sqlite3 import tempfile +import tenacity + # Python 3.8 MutableMapping = getattr(collections, 'abc', collections).MutableMapping @@ -71,6 +73,13 @@ def memoize(permanent_cache=None): return decorator +_retry = tenacity.retry( + retry=tenacity.retry_if_exception_type(sqlite3.OperationalError), + wait=tenacity.wait_exponential(min=0.1, max=2, multiplier=1), + stop=tenacity.stop_after_delay(5), + reraise=True) + + class PersistentDict(MutableMapping): DBPATH = os.path.join(tempfile.gettempdir(), 'sushy-emulator') @@ -88,8 +97,6 @@ class PersistentDict(MutableMapping): '(key blob primary key not null, value blob not null)' ) - self.update(dict(self)) - @staticmethod def encode(obj): return pickle.dumps(obj) @@ -103,16 +110,10 @@ class PersistentDict(MutableMapping): if not self._dbpath: raise TypeError('Dict is not yet persistent') - connection = sqlite3.connect(self._dbpath) - - try: + with sqlite3.connect(self._dbpath) as connection: yield connection.cursor() - connection.commit() - - finally: - connection.close() - + @_retry def __getitem__(self, key): key = self.encode(key) @@ -128,6 +129,7 @@ class PersistentDict(MutableMapping): return self.decode(value[0]) + @_retry def __setitem__(self, key, value): key = self.encode(key) value = self.encode(value) @@ -138,23 +140,19 @@ class PersistentDict(MutableMapping): (key, value) ) + @_retry def __delitem__(self, key): key = self.encode(key) with self.connection() as cursor: - cursor.execute( - 'select count(*) from cache where key=?', - (key,) - ) - - if cursor.fetchone()[0] == 0: - raise KeyError(key) - cursor.execute( 'delete from cache where key=?', (key,) ) + if not cursor.rowcount: + raise KeyError(key) + @_retry def __iter__(self): with self.connection() as cursor: cursor.execute( @@ -165,6 +163,7 @@ class PersistentDict(MutableMapping): for r in records: yield self.decode(r[0]) + @_retry def __len__(self): with self.connection() as cursor: cursor.execute( diff --git a/sushy_tools/tests/unit/emulator/test_memoize.py b/sushy_tools/tests/unit/emulator/test_memoize.py index b3e3b1cb..3a1b0c2c 100644 --- a/sushy_tools/tests/unit/emulator/test_memoize.py +++ b/sushy_tools/tests/unit/emulator/test_memoize.py @@ -12,6 +12,9 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. + +import pickle +import sqlite3 from unittest import mock from oslotest import base @@ -82,112 +85,145 @@ class MemoizeTestCase(base.BaseTestCase): self.assertEqual(0, driver.call_count) +@mock.patch.object(sqlite3, 'connect', autospec=True) class PersistentDictTestCase(base.BaseTestCase): - @mock.patch.object(memoize, 'sqlite3', autospec=True) def test_make_permanent(self, mock_sqlite3): pd = memoize.PersistentDict() - pd.make_permanent('/', 'file') + mock_sqlite3.assert_called_once_with('/file.sqlite') - mock_sqlite3.connect.assert_called_with('/file.sqlite') - - @mock.patch.object(memoize, 'pickle', autospec=True) - def test_encode(self, mock_pickle): + def test_encode(self, mock_sqlite3): pd = memoize.PersistentDict() + value = pd.encode({1: '2'}) + self.assertEqual(pickle.dumps({1: '2'}), value) - pd.encode({1: '2'}) - - mock_pickle.dumps.assert_called_once_with({1: '2'}) - - @mock.patch.object(memoize, 'pickle', autospec=True) - def test_decode(self, mock_pickle): + def test_decode(self, mock_sqlite3): pd = memoize.PersistentDict() + value = pd.decode(pickle.dumps({1: '2'})) + self.assertEqual({1: '2'}, value) - pd.decode('blob') - - mock_pickle.loads.assert_called_once_with('blob') - - @mock.patch.object(memoize, 'pickle', autospec=True) - @mock.patch.object(memoize, 'sqlite3', autospec=True) - def test___getitem__(self, mock_sqlite3, mock_pickle): + def test___getitem__(self, mock_sqlite3): pd = memoize.PersistentDict() pd.make_permanent('/', 'file') - mock_pickle.dumps.return_value = 'pickled-key' - mock_connection = mock_sqlite3.connect.return_value - mock_cursor = mock_connection.cursor.return_value - mock_cursor.fetchone.return_value = ['pickled-value'] + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.fetchone.return_value = [pickle.dumps('pickled-value')] - pd[1] + result = pd[1] + self.assertEqual('pickled-value', result) mock_cursor.execute.assert_called_with( - 'select value from cache where key=?', ('pickled-key',)) - mock_pickle.loads.assert_called_once_with('pickled-value') + 'select value from cache where key=?', (pickle.dumps(1),)) - @mock.patch.object(memoize, 'pickle', autospec=True) - @mock.patch.object(memoize, 'sqlite3', autospec=True) - def test___setitem__(self, mock_sqlite3, mock_pickle): + def test___getitem__retries(self, mock_sqlite3): pd = memoize.PersistentDict() pd.make_permanent('/', 'file') - mock_pickle.dumps.side_effect = [ - 'pickled-key', 'pickled-value'] + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.execute.side_effect = [ + sqlite3.OperationalError, + None + ] + mock_cursor.fetchone.return_value = [pickle.dumps('pickled-value')] - mock_connection = mock_sqlite3.connect.return_value - mock_cursor = mock_connection.cursor.return_value + result = pd[1] + self.assertEqual('pickled-value', result) + + mock_cursor.execute.assert_called_with( + 'select value from cache where key=?', (pickle.dumps(1),)) + self.assertEqual(2, mock_cursor.execute.call_count) + + def test___setitem__(self, mock_sqlite3): + pd = memoize.PersistentDict() + pd.make_permanent('/', 'file') + + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + + pd[1] = 2 + + mock_cursor.execute.assert_called_once_with( + 'insert or replace into cache values (?, ?)', + (pickle.dumps(1), pickle.dumps(2))) + + def test___setitem__retries(self, mock_sqlite3): + pd = memoize.PersistentDict() + pd.make_permanent('/', 'file') + + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.execute.side_effect = [ + sqlite3.OperationalError, + None + ] pd[1] = 2 mock_cursor.execute.assert_called_with( 'insert or replace into cache values (?, ?)', - ('pickled-key', 'pickled-value')) + (pickle.dumps(1), pickle.dumps(2))) + self.assertEqual(2, mock_cursor.execute.call_count) - @mock.patch.object(memoize, 'pickle', autospec=True) - @mock.patch.object(memoize, 'sqlite3', autospec=True) - def test___delitem__(self, mock_sqlite3, mock_pickle): + def test___delitem__(self, mock_sqlite3): pd = memoize.PersistentDict() pd.make_permanent('/', 'file') - mock_pickle.dumps.return_value = 'pickled-key' - mock_connection = mock_sqlite3.connect.return_value - mock_cursor = mock_connection.cursor.return_value + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() del pd[1] - mock_cursor.execute.assert_called_with( - 'delete from cache where key=?', ('pickled-key',)) + mock_cursor.execute.assert_called_once_with( + 'delete from cache where key=?', (pickle.dumps(1),)) - @mock.patch.object(memoize, 'pickle', autospec=True) - @mock.patch.object(memoize, 'sqlite3', autospec=True) - def test___iter__(self, mock_sqlite3, mock_pickle): + def test___delitem__fails(self, mock_sqlite3): pd = memoize.PersistentDict() pd.make_permanent('/', 'file') - mock_pickle.dumps.return_value = 'pickled-key' - mock_connection = mock_sqlite3.connect.return_value - mock_cursor = mock_connection.cursor.return_value - mock_cursor.fetchall.return_value = [['pickled-key']] + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.rowcount = 0 - for x in pd: - x += x + def _del(): + del pd[1] - mock_cursor.execute.assert_called_with('select key from cache') - mock_pickle.loads.assert_called_once_with('pickled-key') + self.assertRaises(KeyError, _del) - @mock.patch.object(memoize, 'pickle', autospec=True) - @mock.patch.object(memoize, 'sqlite3', autospec=True) - def test___len__(self, mock_sqlite3, mock_pickle): + mock_cursor.execute.assert_called_once_with( + 'delete from cache where key=?', (pickle.dumps(1),)) + + def test___iter__(self, mock_sqlite3): pd = memoize.PersistentDict() pd.make_permanent('/', 'file') - mock_connection = mock_sqlite3.connect.return_value - mock_cursor = mock_connection.cursor.return_value + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.fetchall.return_value = [[pickle.dumps('pickled-key')]] - expected = 1 + self.assertEqual(['pickled-key'], list(iter(pd))) - mock_cursor.fetchone.return_value = [expected] + mock_cursor.execute.assert_called_once_with('select key from cache') - self.assertEqual(expected, len(pd)) + def test___len__(self, mock_sqlite3): + pd = memoize.PersistentDict() + pd.make_permanent('/', 'file') - mock_cursor.execute.assert_called_with('select count(*) from cache') + mock_conn = mock_sqlite3.return_value.__enter__.return_value + mock_cursor = mock_conn.cursor.return_value + mock_cursor.execute.reset_mock() + mock_cursor.fetchone.return_value = [42] + + self.assertEqual(42, len(pd)) + + mock_cursor.execute.assert_called_once_with( + 'select count(*) from cache')