fix(config): share one ConfigManager across the process (#691)
* fix(config): share one ConfigManager across the process main(), the tray, and the sound-effects check each constructed their own ConfigManager. Every instance loads the whole config into memory at construction time and save_config() writes that whole cache back, so any save from a stale instance silently reverts what another instance saved in the meantime — no race needed, the second save just wins with old values. The disk-read workarounds in main (_should_append_trailing_space) already existed to dodge exactly this. Add get_shared_config_manager() and route the three call sites through it. It keeps ConfigManager constructible for tests, and the tray/dialog pair already shared one instance, so this extends that arrangement to the rest of the process. Tests: a regression test runs the two-instance scenario through the accessor, and a source guard fails if application code constructs ConfigManager directly again. conftest drops the shared instance between tests, since it would otherwise be created under whichever mocks the first caller had active and leak them into later tests. The two single-instance tests now patch acquire_lock itself: they used patch.dict(sys.modules, ...), which main()'s "from . import" ignores once an earlier test has imported the real module — they only passed through an unrelated mock leaking into the tray's shortcut setup, which this change stops. * fix(config): lock the shared accessor and prove the revert it prevents Review follow-ups on #691: - guard the lazy create with a lock, so two threads racing the first call cannot each build a ConfigManager and reintroduce #689 - take both handles before the first save in the accessor test, and assert identity there — the old order let a fresh ConfigManager pass - add a characterization test: two direct ConfigManager instances still revert each other, which is the footgun the accessor exists to hide - patch get_shared_config_manager in test_main and test_audio_feedback instead of the class, so a leftover singleton cannot skip the mock - close the ExitStack in _boot_under_patches when main() raises; the caller only closes a stack it received, so a failure there leaked those patches into every later test * fix(audio): route the tone lookup through the shared ConfigManager The tone picker merged on main (#707) constructs its own ConfigManager, which is exactly the stale-cache overwrite this branch removes; the new source guard in test_shared_config_manager.py flags it.
This commit is contained in:
@@ -29,9 +29,10 @@ def _should_append_trailing_space() -> bool:
|
||||
"""Return whether completed transcriptions should get a trailing space.
|
||||
|
||||
Reads config.json from disk on each call so Settings toggles take effect
|
||||
immediately. TrayIndicator and main() each construct their own
|
||||
ConfigManager, so an in-memory read from main's instance would miss
|
||||
Settings writes (same pattern as TextInjector._should_copy_to_clipboard).
|
||||
immediately. Historically TrayIndicator and main() each constructed their
|
||||
own ConfigManager, so an in-memory read would miss Settings writes; the
|
||||
instance is shared now, but the disk read stays as the conservative path
|
||||
(same pattern as TextInjector._should_copy_to_clipboard).
|
||||
"""
|
||||
try:
|
||||
import json
|
||||
@@ -325,7 +326,7 @@ def main():
|
||||
from .text_injection import text_injector
|
||||
from .ui import tray_indicator
|
||||
from .ui.action_handler import ActionHandler
|
||||
from .ui.config_manager import ConfigManager
|
||||
from .ui.config_manager import get_shared_config_manager
|
||||
from .ui.logging_manager import initialize_logging
|
||||
|
||||
# Initialize logging manager early
|
||||
@@ -342,7 +343,7 @@ def main():
|
||||
except Exception as e:
|
||||
logger.debug(f"Could not start IBus daemon: {e}")
|
||||
|
||||
config_manager = ConfigManager()
|
||||
config_manager = get_shared_config_manager()
|
||||
saved_settings = config_manager.get_settings().get("speech_recognition", {})
|
||||
audio_settings = config_manager.get_settings().get("audio", {})
|
||||
|
||||
@@ -501,8 +502,7 @@ def main():
|
||||
|
||||
text_to_inject = capitalize_sentences(text_to_inject)
|
||||
|
||||
# Read from disk so the Settings toggle applies without restart
|
||||
# (main and tray each own a ConfigManager instance).
|
||||
# Read from disk so the Settings toggle applies without restart.
|
||||
append_trailing_space = _should_append_trailing_space()
|
||||
|
||||
if append_trailing_space:
|
||||
|
||||
@@ -59,18 +59,18 @@ ERROR_SOUND = _resource_manager.get_sound_path("error")
|
||||
|
||||
def _is_sound_effects_enabled() -> bool:
|
||||
try:
|
||||
from .config_manager import ConfigManager
|
||||
from .config_manager import get_shared_config_manager
|
||||
|
||||
return ConfigManager().is_sound_effects_enabled()
|
||||
return get_shared_config_manager().is_sound_effects_enabled()
|
||||
except Exception:
|
||||
return True
|
||||
|
||||
|
||||
def _resolved_tone() -> str:
|
||||
try:
|
||||
from .config_manager import ConfigManager
|
||||
from .config_manager import get_shared_config_manager
|
||||
|
||||
return ConfigManager().get_sound_effects_tone()
|
||||
return get_shared_config_manager().get_sound_effects_tone()
|
||||
except Exception:
|
||||
return DEFAULT_SOUND_EFFECT_TONE
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ import copy
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import threading
|
||||
from typing import Any, Optional
|
||||
|
||||
from ..utils.paths import config_dir
|
||||
@@ -487,3 +488,27 @@ class ConfigManager:
|
||||
self._update_dict_recursive(target[key], value)
|
||||
else:
|
||||
target[key] = value
|
||||
|
||||
|
||||
# The one instance the application should share. Every ConfigManager caches the
|
||||
# whole config in memory and save_config() writes that whole cache back, so two
|
||||
# live instances silently revert each other's saves — the second save wins with
|
||||
# whatever stale values it still holds. Tests may still construct ConfigManager
|
||||
# directly; application code goes through this accessor.
|
||||
_shared_instance: Optional[ConfigManager] = None
|
||||
_shared_instance_lock = threading.Lock()
|
||||
|
||||
|
||||
def get_shared_config_manager() -> ConfigManager:
|
||||
"""Return the process-wide ConfigManager, creating it on first use.
|
||||
|
||||
Creation is locked: __init__ does makedirs() plus a full load_config(),
|
||||
so two threads racing the first call would otherwise each build their own
|
||||
instance and reintroduce the very overwrite this accessor prevents.
|
||||
"""
|
||||
global _shared_instance
|
||||
if _shared_instance is None:
|
||||
with _shared_instance_lock:
|
||||
if _shared_instance is None:
|
||||
_shared_instance = ConfigManager()
|
||||
return _shared_instance
|
||||
|
||||
@@ -40,7 +40,7 @@ from ..suspend_handler import SuspendHandler
|
||||
from ..utils.resource_manager import ResourceManager
|
||||
from ..utils.update_checker import ReleaseInfo
|
||||
from ..utils.update_monitor import UpdateMonitor
|
||||
from .config_manager import ConfigManager
|
||||
from .config_manager import get_shared_config_manager
|
||||
from .keyboard_backends import DEFAULT_SHORTCUT, DEFAULT_SHORTCUT_MODE
|
||||
from .keyboard_shortcuts import KeyboardShortcutManager
|
||||
from .settings_dialog import SettingsDialog
|
||||
@@ -104,7 +104,9 @@ class TrayIndicator:
|
||||
"""
|
||||
self.speech_engine = speech_engine
|
||||
self.text_injector = text_injector
|
||||
self.config_manager = ConfigManager() # Added: Initialize ConfigManager
|
||||
# Shared with main() and the settings dialog: separate instances would
|
||||
# overwrite each other's saves with stale in-memory copies.
|
||||
self.config_manager = get_shared_config_manager()
|
||||
self._syncing_autostart_menu = False
|
||||
|
||||
# Get configured shortcut and mode from config
|
||||
|
||||
Reference in New Issue
Block a user