From 0a9927d18e33bfefdda4365d4563ec27b1d02f55 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Tue, 11 Jan 2022 16:17:56 +0100 Subject: [PATCH] Avoid locking the database for non-SQLite backends (#63847) * Avoid locking the database for non-SQLite backends Currently we only have a lock implementation for SQLite. Just return success for all other databases as they are not expected to store data in the config directory and the caller can assume that a backup can be safely taken. This fixes `RuntimeError: generator didn't yield` errors when creating a backup with the current Supervisor dev builds. --- homeassistant/components/recorder/__init__.py | 16 +++++++++-- homeassistant/components/recorder/util.py | 27 +++++++++---------- tests/components/recorder/test_init.py | 25 ++++++++++++++++- tests/components/recorder/test_util.py | 2 +- 4 files changed, 52 insertions(+), 18 deletions(-) diff --git a/homeassistant/components/recorder/__init__.py b/homeassistant/components/recorder/__init__.py index 1589e767fa7..410ad165ce1 100644 --- a/homeassistant/components/recorder/__init__.py +++ b/homeassistant/components/recorder/__init__.py @@ -78,7 +78,7 @@ from .util import ( session_scope, setup_connection_for_dialect, validate_or_move_away_sqlite_database, - write_lock_db, + write_lock_db_sqlite, ) _LOGGER = logging.getLogger(__name__) @@ -870,7 +870,7 @@ class Recorder(threading.Thread): def _async_set_database_locked(task: DatabaseLockTask): task.database_locked.set() - with write_lock_db(self): + with write_lock_db_sqlite(self): # Notify that lock is being held, wait until database can be used again. self.hass.add_job(_async_set_database_locked, task) while not task.database_unlock.wait(timeout=DB_LOCK_QUEUE_CHECK_TIMEOUT): @@ -1057,6 +1057,12 @@ class Recorder(threading.Thread): async def lock_database(self) -> bool: """Lock database so it can be backed up safely.""" + if not self.engine or self.engine.dialect.name != "sqlite": + _LOGGER.debug( + "Not a SQLite database or not connected, locking not necessary" + ) + return True + if self._database_lock_task: _LOGGER.warning("Database already locked") return False @@ -1080,6 +1086,12 @@ class Recorder(threading.Thread): Returns true if database lock has been held throughout the process. """ + if not self.engine or self.engine.dialect.name != "sqlite": + _LOGGER.debug( + "Not a SQLite database or not connected, unlocking not necessary" + ) + return True + if not self._database_lock_task: _LOGGER.warning("Database currently not locked") return False diff --git a/homeassistant/components/recorder/util.py b/homeassistant/components/recorder/util.py index 734694b8224..9d5ee6f29d3 100644 --- a/homeassistant/components/recorder/util.py +++ b/homeassistant/components/recorder/util.py @@ -462,22 +462,21 @@ def perodic_db_cleanups(instance: Recorder): @contextmanager -def write_lock_db(instance: Recorder): +def write_lock_db_sqlite(instance: Recorder): """Lock database for writes.""" - if instance.engine.dialect.name == "sqlite": - with instance.engine.connect() as connection: - # Execute sqlite to create a wal checkpoint - # This is optional but makes sure the backup is going to be minimal - connection.execute(text("PRAGMA wal_checkpoint(TRUNCATE)")) - # Create write lock - _LOGGER.debug("Lock database") - connection.execute(text("BEGIN IMMEDIATE;")) - try: - yield - finally: - _LOGGER.debug("Unlock database") - connection.execute(text("END;")) + with instance.engine.connect() as connection: + # Execute sqlite to create a wal checkpoint + # This is optional but makes sure the backup is going to be minimal + connection.execute(text("PRAGMA wal_checkpoint(TRUNCATE)")) + # Create write lock + _LOGGER.debug("Lock database") + connection.execute(text("BEGIN IMMEDIATE;")) + try: + yield + finally: + _LOGGER.debug("Unlock database") + connection.execute(text("END;")) def async_migration_in_progress(hass: HomeAssistant) -> bool: diff --git a/tests/components/recorder/test_init.py b/tests/components/recorder/test_init.py index ef5ebff44c4..2bc0109e1b5 100644 --- a/tests/components/recorder/test_init.py +++ b/tests/components/recorder/test_init.py @@ -3,6 +3,7 @@ import asyncio from datetime import datetime, timedelta import sqlite3 +import threading from unittest.mock import patch import pytest @@ -1204,12 +1205,34 @@ async def test_database_lock_timeout(hass): """Test locking database timeout when recorder stopped.""" await async_init_recorder_component(hass) hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) - await hass.async_block_till_done() instance: Recorder = hass.data[DATA_INSTANCE] + + class BlockQueue(recorder.RecorderTask): + event: threading.Event = threading.Event() + + def run(self, instance: Recorder) -> None: + self.event.wait() + + block_task = BlockQueue() + instance.queue.put(block_task) with patch.object(recorder, "DB_LOCK_TIMEOUT", 0.1): try: with pytest.raises(TimeoutError): await instance.lock_database() finally: instance.unlock_database() + block_task.event.set() + + +async def test_database_lock_without_instance(hass): + """Test database lock doesn't fail if instance is not initialized.""" + await async_init_recorder_component(hass) + hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) + + instance: Recorder = hass.data[DATA_INSTANCE] + with patch.object(instance, "engine", None): + try: + assert await instance.lock_database() + finally: + assert instance.unlock_database() diff --git a/tests/components/recorder/test_util.py b/tests/components/recorder/test_util.py index fa449aefefc..ec74ea73975 100644 --- a/tests/components/recorder/test_util.py +++ b/tests/components/recorder/test_util.py @@ -570,7 +570,7 @@ async def test_write_lock_db(hass, tmp_path): instance = hass.data[DATA_INSTANCE] - with util.write_lock_db(instance): + with util.write_lock_db_sqlite(instance): # Database should be locked now, try writing SQL command with instance.engine.connect() as connection: with pytest.raises(OperationalError):