Repository: miurahr/aqtinstall

Issue:
Setting class does not always hold the user's settings
**Describe the bug**
I'm working on some changes in aqt, and it seems to me that the `Settings` object is not really a singleton, but several instances exist of it. Only one of them has the user's settings.

**To Reproduce**
Add a line `print(f"Trusted mirrors from {self}")` in the implementation of the `trusted_mirrors` property in `helper.py`. The run aqt. You'll see that it prints different values for self. If you also have a configuration file with some custom settings, you'll see that they are often missing.

I'm trying to reimplement the SettingsClass as a singleton as described [here](https://stackoverflow.com/questions/6760685/creating-a-singleton-in-python) but I must be doing something wrong and so far I haven't had any luck in getting this work properly.

Discussion:
mardy: Thanks, I'll try to find some time to write a test today, but my suspicion is that there are two different issues:
1. Settings is not a singleton
2. Custom settings are not loaded by the installer: it always loads the default values: https://github.com/miurahr/aqtinstall/blob/master/aqt/installer.py#L1087

But if this MR works, then I guess that the `load_settings` line could just be removed from the installer, and everything would work fine.
mardy: Oh, after reading a bit the documentation, I think that this approach won't help: the problem is that the `multiprocess` module creates real separate processes, so it's obvious that they'll have a different Settings instance each.

So I guess there are two ways forward:
1. Keep the multiprocess approach, accept the fact that we'll have different instances of the Settings, and reload them from the config file in each process.
2. Use a singleton and switch to using the [ThreadPoolExecutor from concurrent.futures](https://docs.python.org/3/library/concurrent.futures.html#threadpoolexecutor)

Using threads means that we hit Python's global interpreter lock, but on the other hand we are not really doing parallel computing work, we are just downloading files. So it might be fine.
ddalcino: > Oh, after reading a bit the documentation, I think that this approach won't help: the problem is that the `multiprocess` module creates real separate processes, so it's obvious that they'll have a different Settings instance each.

This is true, unless we start sharing memory between processes, and that looks difficult to set up with `multiprocessing`.

> So I guess there are two ways forward:
> 
> 1. Keep the multiprocess approach, accept the fact that we'll have different instances of the Settings, and reload them from the config file in each process.

I vote for this idea. It's easy to pass the path to `settings.ini` in an argument list to each process. I don't think the Settings object is ever modified after it is loaded, so this should work fine.

> Using threads means that we hit Python's global interpreter lock, but on the other hand we are not really doing parallel computing work, we are just downloading files. So it might be fine.

IIRC, .7z archive extraction is CPU-bound, and the multiprocessing library gets us some speed benefits when compared to the multithreading library. See comment: https://github.com/miurahr/aqtinstall/issues/264#issuecomment-864182224

I had forgotten about #264. I think any attempt at fixing this will have to be very careful not to reintroduce the bug. I don't think we ever wrote a test for it.
miurahr: Another point we can improve is `Settings` global variable that is not share among `multiprocessing`.

Patch:
diff --git a/aqt/helper.py b/aqt/helper.py
index 6208a89..2e6dee7 100644
--- a/aqt/helper.py
+++ b/aqt/helper.py
@@ -27,6 +27,7 @@ import os
 import posixpath
 import secrets
 import sys
+import threading
 from logging import Handler, getLogger
 from logging.handlers import QueueListener
 from pathlib import Path
@@ -321,13 +322,30 @@ class SettingsClass:
     """
     Class to hold configuration and settings.
     Actual values are stored in 'settings.ini' file.
-    It also holds a combinations database.
+    It also holds a `combinations` database.
     """
 
+    # this class is Borg
+    _shared_state: Dict[str, Any] = {
+        "config": None,
+        "configfile": None,
+        "loggingconf": None,
+        "_combinations": None,
+        "_lock": threading.Lock(),
+    }
+
+    def __new__(cls, *p, **k):
+        self = object.__new__(cls, *p, **k)
+        self.__dict__ = cls._shared_state
+        return self
+
     def __init__(self):
-        self.config = MyConfigParser()
-        self.configfile = os.path.join(os.path.dirname(__file__), "settings.ini")
-        self.loggingconf = os.path.join(os.path.dirname(__file__), "logging.ini")
+        if self.config is None:
+            with self._lock:
+                if self.config is None:
+                    self.config = MyConfigParser()
+                    self.configfile = os.path.join(os.path.dirname(__file__), "settings.ini")
+                    self.loggingconf = os.path.join(os.path.dirname(__file__), "logging.ini")
 
     def load_settings(self, file: Optional[Union[str, TextIO]] = None) -> None:
         with open(
@@ -478,6 +496,5 @@ Settings = SettingsClass()
 def setup_logging(env_key="LOG_CFG"):
     config = os.getenv(env_key, None)
     if config is not None and os.path.exists(config):
-        logging.config.fileConfig(config)
-    else:
-        logging.config.fileConfig(Settings.loggingconf)
+        Settings.loggingconf = config
+    logging.config.fileConfig(Settings.loggingconf)
diff --git a/aqt/installer.py b/aqt/installer.py
index 33aec9a..1a16cad 100644
--- a/aqt/installer.py
+++ b/aqt/installer.py
@@ -1099,7 +1099,7 @@ def run_installer(archives: List[QtPackage], base_dir: str, sevenzip: Optional[s
     #
     tasks = []
     for arc in archives:
-        tasks.append((arc, base_dir, sevenzip, queue, archive_dest, keep))
+        tasks.append((arc, base_dir, sevenzip, queue, archive_dest, Settings.configfile, keep))
     ctx = multiprocessing.get_context("spawn")
     if is_64bit():
         pool = ctx.Pool(Settings.concurrency, init_worker_sh, (), 4)
@@ -1154,8 +1154,8 @@ def installer(
     command: Optional[str],
     queue: multiprocessing.Queue,
     archive_dest: Path,
-    keep: bool = False,
-    response_timeout: Optional[int] = None,
+    settings_ini: str,
+    keep: bool,
 ):
     """
     Installer function to download archive files and extract it.
@@ -1165,10 +1165,9 @@ def installer(
     base_url = qt_package.base_url
     archive: Path = archive_dest / qt_package.archive
     start_time = time.perf_counter()
-    # set defaults
-    Settings.load_settings()
-    # set logging
-    setup_logging()  # XXX: why need to load again?
+    Settings.load_settings(file=settings_ini)
+    # setup queue logger
+    setup_logging()
     qh = QueueHandler(queue)
     logger = getLogger()
     for handler in logger.handlers:
@@ -1176,10 +1175,7 @@ def installer(
         logger.removeHandler(handler)
     logger.addHandler(qh)
     #
-    if response_timeout is None:
-        timeout = (Settings.connection_timeout, Settings.response_timeout)
-    else:
-        timeout = (Settings.connection_timeout, response_timeout)
+    timeout = (Settings.connection_timeout, Settings.response_timeout)
     hash = get_hash(qt_package.archive_path, algorithm="sha256", timeout=timeout)
 
     def download_bin(_base_url):
diff --git a/ci/settings.ini b/ci/settings.ini
index ae1f837..9aac817 100644
--- a/ci/settings.ini
+++ b/ci/settings.ini
@@ -9,7 +9,10 @@ baseurl: https://download.qt.io
 connection_timeout: 30
 response_timeout: 30
 max_retries: 5
+max_retries_on_connection_error: 5
 retry_backoff: 0.1
+max_retries_on_checksum_error: 5
+max_retries_to_retrieve_hash: 5
 
 [mirrors]
 blacklist:


Test patch:
diff --git a/tests/test_install.py b/tests/test_install.py
index 79945b4..fc373ff 100644
--- a/tests/test_install.py
+++ b/tests/test_install.py
@@ -1215,6 +1215,8 @@ def test_install_installer_archive_extraction_err(monkeypatch):
             command="some_nonexistent_7z_extractor",
             queue=MockMultiprocessingManager.Queue(),
             archive_dest=Path(temp_dir),
+            settings_ini=Settings.configfile,
+            keep=False,
         )
     assert err.type == ArchiveExtractionError
     err_msg = format(err.value).rstrip()