Windows registry: improve search efficiency & error reporting (spack#…

…41720)

* Registry queries can fail due to simultaneous access from other
  processes. Wrap some of these accesses and retry when this may
  be the cause (the generated exceptions don't allow pinpointing
  this as the reason, but we add logic to identify cases which
  are definitely unrecoverable, and retry if it is not one of
  these).
* Add make recursion optioal for most registry search functions;
  disable recursive search in case where it was originally always
  recursive.

lib/spack/spack/operating_systems/windows_os.py

@@ -9,6 +9,8 @@
 import platform
 import subprocess
 
+from llnl.util import tty
+
 from spack.error import SpackError
 from spack.util import windows_registry as winreg
 from spack.version import Version
@@ -83,11 +85,50 @@ def compiler_search_paths(self):
                     os.path.join(str(os.getenv("ONEAPI_ROOT")), "compiler", "*", "windows", "bin")
                 )
             )
+
         # Second strategy: Find MSVC via the registry
-        msft = winreg.WindowsRegistryView(
-            "SOFTWARE\\WOW6432Node\\Microsoft", winreg.HKEY.HKEY_LOCAL_MACHINE
-        )
-        vs_entries = msft.find_subkeys(r"VisualStudio_.*")
+        def try_query_registry(retry=False):
+            winreg_report_error = lambda e: tty.debug(
+                'Windows registry query on "SOFTWARE\\WOW6432Node\\Microsoft"'
+                f"under HKEY_LOCAL_MACHINE: {str(e)}"
+            )
+            try:
+                # Registry interactions are subject to race conditions, etc and can generally
+                # be flakey, do this in a catch block to prevent reg issues from interfering
+                # with compiler detection
+                msft = winreg.WindowsRegistryView(
+                    "SOFTWARE\\WOW6432Node\\Microsoft", winreg.HKEY.HKEY_LOCAL_MACHINE
+                )
+                return msft.find_subkeys(r"VisualStudio_.*", recursive=False)
+            except OSError as e:
+                # OSErrors propagated into caller by Spack's registry module are expected
+                # and indicate a known issue with the registry query
+                # i.e. user does not have permissions or the key/value
+                # doesn't exist
+                winreg_report_error(e)
+                return []
+            except winreg.InvalidRegistryOperation as e:
+                # Other errors raised by the Spack's reg module indicate
+                # an unexpected error type, and are handled specifically
+                # as the underlying cause is difficult/impossible to determine
+                # without manually exploring the registry
+                # These errors can also be spurious (race conditions)
+                # and may resolve on re-execution of the query
+                # or are permanent (specific types of permission issues)
+                # but the registry raises the same exception for all types of
+                # atypical errors
+                if retry:
+                    winreg_report_error(e)
+                return []
+
+        vs_entries = try_query_registry()
+        if not vs_entries:
+            # Occasional spurious race conditions can arise when reading the MS reg
+            # typically these race conditions resolve immediately and we can safely
+            # retry the reg query without waiting
+            # Note: Winreg does not support locking
+            vs_entries = try_query_registry(retry=True)
+
         vs_paths = []
 
         def clean_vs_path(path):
@@ -99,11 +140,8 @@ def clean_vs_path(path):
                 val = entry.get_subkey("Capabilities").get_value("ApplicationDescription").value
                 vs_paths.append(clean_vs_path(val))
             except FileNotFoundError as e:
-                if hasattr(e, "winerror"):
-                    if e.winerror == 2:
-                        pass
-                    else:
-                        raise
+                if hasattr(e, "winerror") and e.winerror == 2:
+                    pass
                 else:
                     raise
 

lib/spack/spack/util/windows_registry.py

@@ -59,49 +59,99 @@ def subkeys(self):
     def hkey(self):
         return self._handle
 
+    @contextmanager
+    def winreg_error_handler(self, name, *args, **kwargs):
+        try:
+            yield
+        except OSError as err:
+            # Expected errors that occur on occasion, these are easily
+            # debug-able and have sufficiently verbose reporting and obvious cause
+            # [WinError 2]: the system cannot find the file specified - lookup item does
+            # not exist
+            # [WinError 5]: Access is denied - user not in key's ACL
+            if hasattr(err, "winerror") and err.winerror in (5, 2):
+                raise err
+            # Other OS errors are more difficult to diagnose, so we wrap them in some extra
+            # reporting
+            raise InvalidRegistryOperation(name, err, *args, **kwargs) from err
+
+    def OpenKeyEx(self, subname, **kwargs):
+        """Convenience wrapper around winreg.OpenKeyEx"""
+        tty.debug(
+            f"[WINREG ACCESS] Accessing Reg Key {self.path}/{subname} with"
+            f" {kwargs.get('access', 'default')} access"
+        )
+        with self.winreg_error_handler("OpenKeyEx", subname, **kwargs):
+            return winreg.OpenKeyEx(self.hkey, subname, **kwargs)
+
+    def QueryInfoKey(self):
+        """Convenience wrapper around winreg.QueryInfoKey"""
+        tty.debug(f"[WINREG ACCESS] Obtaining key,value information from key {self.path}")
+        with self.winreg_error_handler("QueryInfoKey"):
+            return winreg.QueryInfoKey(self.hkey)
+
+    def EnumKey(self, index):
+        """Convenience wrapper around winreg.EnumKey"""
+        tty.debug(
+            "[WINREG ACCESS] Obtaining name of subkey at index "
+            f"{index} from registry key {self.path}"
+        )
+        with self.winreg_error_handler("EnumKey", index):
+            return winreg.EnumKey(self.hkey, index)
+
+    def EnumValue(self, index):
+        """Convenience wrapper around winreg.EnumValue"""
+        tty.debug(
+            f"[WINREG ACCESS] Obtaining value at index {index} from registry key {self.path}"
+        )
+        with self.winreg_error_handler("EnumValue", index):
+            return winreg.EnumValue(self.hkey, index)
+
+    def QueryValueEx(self, name, **kwargs):
+        """Convenience wrapper around winreg.QueryValueEx"""
+        tty.debug(f"[WINREG ACCESS] Obtaining value {name} from registry key {self.path}")
+        with self.winreg_error_handler("QueryValueEx", name, **kwargs):
+            return winreg.QueryValueEx(self.hkey, name, **kwargs)
+
     def __str__(self):
         return self.name
 
     def _gather_subkey_info(self):
         """Composes all subkeys into a list for access"""
         if self._keys:
             return
-        sub_keys, _, _ = winreg.QueryInfoKey(self.hkey)
+        sub_keys, _, _ = self.QueryInfoKey()
         for i in range(sub_keys):
-            sub_name = winreg.EnumKey(self.hkey, i)
+            sub_name = self.EnumKey(i)
             try:
-                sub_handle = winreg.OpenKeyEx(self.hkey, sub_name, access=winreg.KEY_READ)
+                sub_handle = self.OpenKeyEx(sub_name, access=winreg.KEY_READ)
                 self._keys.append(RegistryKey(os.path.join(self.path, sub_name), sub_handle))
             except OSError as e:
-                if hasattr(e, "winerror"):
-                    if e.winerror == 5:
-                        # This is a permission error, we can't read this key
-                        # move on
-                        pass
-                    else:
-                        raise
+                if hasattr(e, "winerror") and e.winerror == 5:
+                    # This is a permission error, we can't read this key
+                    # move on
+                    pass
                 else:
                     raise
 
     def _gather_value_info(self):
         """Compose all values for this key into a dict of form value name: RegistryValue Object"""
         if self._values:
             return
-        _, values, _ = winreg.QueryInfoKey(self.hkey)
+        _, values, _ = self.QueryInfoKey()
         for i in range(values):
-            value_name, value_data, _ = winreg.EnumValue(self.hkey, i)
-            self._values[value_name] = RegistryValue(value_name, value_data, self.hkey)
+            value_name, value_data, _ = self.EnumValue(i)
+            self._values[value_name] = RegistryValue(value_name, value_data, self)
 
     def get_subkey(self, sub_key):
         """Returns subkey of name sub_key in a RegistryKey objects"""
         return RegistryKey(
-            os.path.join(self.path, sub_key),
-            winreg.OpenKeyEx(self.hkey, sub_key, access=winreg.KEY_READ),
+            os.path.join(self.path, sub_key), self.OpenKeyEx(sub_key, access=winreg.KEY_READ)
         )
 
     def get_value(self, val_name):
         """Returns value associated with this key in RegistryValue object"""
-        return RegistryValue(val_name, winreg.QueryValueEx(self.hkey, val_name)[0], self.hkey)
+        return RegistryValue(val_name, self.QueryValueEx(val_name)[0], self)
 
 
 class _HKEY_CONSTANT(RegistryKey):
@@ -197,10 +247,7 @@ def __bool__(self):
 
     def _load_key(self):
         try:
-            self._reg = RegistryKey(
-                os.path.join(str(self.root), self.key),
-                winreg.OpenKeyEx(self.root.hkey, self.key, access=winreg.KEY_READ),
-            )
+            self._reg = self.root.get_subkey(self.key)
         except FileNotFoundError as e:
             if sys.platform == "win32" and e.winerror == 2:
                 self._reg = -1
@@ -210,7 +257,7 @@ def _load_key(self):
 
     def _valid_reg_check(self):
         if self.reg == -1:
-            tty.debug("Cannot perform operation for nonexistent key %s" % self.key)
+            tty.debug(f"[WINREG ACCESS] Cannot perform operation for nonexistent key {self.key}")
             return False
         return True
 
@@ -227,19 +274,19 @@ def reg(self):
     def get_value(self, value_name):
         """Return registry value corresponding to provided argument (if it exists)"""
         if not self._valid_reg_check():
-            raise RegistryError("Cannot query value from invalid key %s" % self.key)
+            raise RegistryError(f"Cannot query value from invalid key {self.key}")
         with self.invalid_reg_ref_error_handler():
             return self.reg.get_value(value_name)
 
     def get_subkey(self, subkey_name):
         if not self._valid_reg_check():
-            raise RegistryError("Cannot query subkey from invalid key %s" % self.key)
+            raise RegistryError(f"Cannot query subkey from invalid key {self.key}")
         with self.invalid_reg_ref_error_handler():
             return self.reg.get_subkey(subkey_name)
 
     def get_subkeys(self):
         if not self._valid_reg_check():
-            raise RegistryError("Cannot query subkeys from invalid key %s" % self.key)
+            raise RegistryError(f"Cannot query subkeys from invalid key {self.key}")
         with self.invalid_reg_ref_error_handler():
             return self.reg.subkeys
 
@@ -252,11 +299,11 @@ def get_matching_subkeys(self, subkey_name):
 
     def get_values(self):
         if not self._valid_reg_check():
-            raise RegistryError("Cannot query values from invalid key %s" % self.key)
+            raise RegistryError(f"Cannot query values from invalid key {self.key}")
         with self.invalid_reg_ref_error_handler():
             return self.reg.values
 
-    def _traverse_subkeys(self, stop_condition, collect_all_matching=False):
+    def _traverse_subkeys(self, stop_condition, collect_all_matching=False, recursive=True):
         """Perform simple BFS of subkeys, returning the key
         that successfully triggers the stop condition.
         Args:
@@ -266,12 +313,13 @@ def _traverse_subkeys(self, stop_condition, collect_all_matching=False):
                             all keys meeting stop condition. If false, once stop
                             condition is met, the key that triggered the condition '
                             is returned.
+            recusrive: boolean value, if True perform a recursive search of subkeys
         Return:
             the key if stop_condition is triggered, or None if not
         """
         collection = []
         if not self._valid_reg_check():
-            raise RegistryError("Cannot query values from invalid key %s" % self.key)
+            raise InvalidKeyError(self.key)
         with self.invalid_reg_ref_error_handler():
             queue = self.reg.subkeys
             for key in queue:
@@ -280,67 +328,49 @@ def _traverse_subkeys(self, stop_condition, collect_all_matching=False):
                         collection.append(key)
                     else:
                         return key
-                queue.extend(key.subkeys)
+                if recursive:
+                    queue.extend(key.subkeys)
             return collection if collection else None
 
-    def _find_subkey_s(self, search_key, collect_all_matching=False):
-        """Retrieve one or more keys regex matching `search_key`.
-        One key will be returned unless `collect_all_matching` is enabled,
-        in which case call matches are returned.
-
-        Args:
-            search_key (str): regex string represeting a subkey name structure
-                              to be matched against.
-                              Cannot be provided alongside `direct_subkey`
-            collect_all_matching (bool): No-op if `direct_subkey` is specified
-        Return:
-            the desired subkey as a RegistryKey object, or none
-        """
-        return self._traverse_subkeys(search_key, collect_all_matching=collect_all_matching)
-
-    def find_subkey(self, subkey_name):
+    def find_subkey(self, subkey_name, recursive=True):
         """Perform a BFS of subkeys until desired key is found
         Returns None or RegistryKey object corresponding to requested key name
 
         Args:
-            subkey_name (str)
+            subkey_name (str): subkey to be searched for
+            recursive (bool):  perform a recursive search
         Return:
             the desired subkey as a RegistryKey object, or none
-
-        For more details, see the WindowsRegistryView._find_subkey_s method docstring
         """
-        return self._find_subkey_s(
-            WindowsRegistryView.KeyMatchConditions.name_matcher(subkey_name)
+        return self._traverse_subkeys(
+            WindowsRegistryView.KeyMatchConditions.name_matcher(subkey_name), recursive=recursive
         )
 
-    def find_matching_subkey(self, subkey_name):
+    def find_matching_subkey(self, subkey_name, recursive=True):
         """Perform a BFS of subkeys until a key matching subkey name regex is found
         Returns None or the first RegistryKey object corresponding to requested key name
 
         Args:
-            subkey_name (str)
+            subkey_name (str): subkey to be searched for
+            recursive (bool):  perform a recursive search
         Return:
             the desired subkey as a RegistryKey object, or none
-
-        For more details, see the WindowsRegistryView._find_subkey_s method docstring
         """
-        return self._find_subkey_s(
-            WindowsRegistryView.KeyMatchConditions.regex_matcher(subkey_name)
+        return self._traverse_subkeys(
+            WindowsRegistryView.KeyMatchConditions.regex_matcher(subkey_name), recursive=recursive
         )
 
-    def find_subkeys(self, subkey_name):
+    def find_subkeys(self, subkey_name, recursive=True):
         """Exactly the same as find_subkey, except this function tries to match
         a regex to multiple keys
 
         Args:
             subkey_name (str)
         Return:
             the desired subkeys as a list of RegistryKey object, or none
-
-        For more details, see the WindowsRegistryView._find_subkey_s method docstring
         """
-        kwargs = {"collect_all_matching": True}
-        return self._find_subkey_s(
+        kwargs = {"collect_all_matching": True, "recursive": recursive}
+        return self._traverse_subkeys(
             WindowsRegistryView.KeyMatchConditions.regex_matcher(subkey_name), **kwargs
         )
 
@@ -366,5 +396,28 @@ def find_value(self, val_name, recursive=True):
                 return key.values[val_name]
 
 
-class RegistryError(RuntimeError):
+class RegistryError(Exception):
+    """RunTime Error concerning the Windows Registry"""
+
+
+class InvalidKeyError(RegistryError):
     """Runtime Error describing issue with invalid key access to Windows registry"""
+
+    def __init__(self, key):
+        message = f"Cannot query invalid key: {key}"
+        super().__init__(message)
+
+
+class InvalidRegistryOperation(RegistryError):
+    """A Runtime Error ecountered when a registry operation is invalid for
+    an indeterminate reason"""
+
+    def __init__(self, name, e, *args, **kwargs):
+        message = (
+            f"Windows registry operations: {name} encountered error: {str(e)}"
+            "\nMethod invoked with parameters:\n"
+        )
+        message += "\n\t".join([f"{k}:{v}" for k, v in kwargs.items()])
+        message += "\n"
+        message += "\n\t".join(args)
+        super().__init__(self, message)