Repository: pyvisa/pyvisa

Issue:
Extra DLL paths from .pyvisarc are never removed from DLL search path
# Problem Description
When specifying extra DLL paths in the `.pyvisarc`, the DLL paths are added to the DLL search path each time a new `ResourceManager` is instantiated, but never removed. Because of this, at some point the DLL search path will become too long for windows to handle, which will result in a WinError 206.

# Steps to Reproduce
1. Have some extra DLL paths in the `.pyvisarc`, for example the Keysight IO libs:<br> `dll_extra_paths: C:\Program Files (x86)\Keysight\IO Libraries Suite\bin`
2. Open and close many `ResourceManager` instances after each other
```
import pyvisa

for i in range(1000):
    print(i)
    rm = pyvisa.ResourceManager()
    rm.close()
```
On my system, at `i = 297` this will result in
```
FileNotFoundError: [WinError 206] The filename or extension is too long:  'C:\\Program Files (x86)\\Keysight\\IO Libraries Suite\\bin'
```

# Possible Fix
In my opinion the issue is here:
https://github.com/pyvisa/pyvisa/blob/6bca99233dcf66742db54d6d410c9840efb18cb5/pyvisa/util.py#L150
The call to `os.add_dll_directory` returns a `os._AddedDllDirectory` object, which needs to be closed according to the python docs for `os`:
>"Remove the directory by calling close() on the returned object or using it in a [with](https://docs.python.org/3/reference/compound_stmts.html#with) statement."
However, the  `os._AddedDllDirectory` is simply discarded here.

I am not yet sure about a fix, do we need to keep track of the `os._AddedDllDirectory` until the `ResourceManger` instance is closed?
Or could we return the path to the DLL library as a string, and later construct a `LibraryPath` object from the path in `get_library_paths`?

Hints from issue comments:
@arr-ee: We should keep track of what we've added already, and short-circuit if we try adding the same thing again. That function is not limited to (even if it's unlikely to be called outside of) creation of `ResourceManager`.

_Ideally_ we would do some eager configuration instead so that we have a single place we execute and store results of all the pathfinding etc, but it's a bit of an undertaking.@MatthieuDartiailh: Do you want to make a PR @arr-ee ?

I agree we should track what we added to avoid duplication and ideally also use `atexit` to ensure we close the objects before exiting.

Solution patch:
diff --git a/CHANGES b/CHANGES
index 4b3d13d4..43deefdd 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4,9 +4,11 @@ PyVISA Changelog
 1.15.0 (unreleased)
 -------------------
 - added support for an optional monitoring interface object to the read_binary_values() and
-    query_binary_values() methods of message-based resources.
+  query_binary_values() methods of message-based resources.
 - added the message_size() helper function to calculate the estimated size of a data
-    transfer, including the header.
+  transfer, including the header.
+- prevent adding duplicate paths to the DLL search path during ResourceManager
+  instantiation #811
 
 1.14.1 (22-11-2023)
 -------------------
diff --git a/pyvisa/util.py b/pyvisa/util.py
index 6dd48d2e..5c571510 100644
--- a/pyvisa/util.py
+++ b/pyvisa/util.py
@@ -23,6 +23,7 @@
 from pathlib import Path
 from types import ModuleType
 from typing import (
+    AbstractSet,
     Any,
     Callable,
     Dict,
@@ -109,6 +110,9 @@ def read_user_library_path() -> Optional[str]:
         return None
 
 
+_ADDED_DLL_PATHS: AbstractSet[str] = set()
+
+
 def add_user_dll_extra_paths() -> Optional[List[str]]:
     """Add paths to search for .dll dependencies on Windows.
 
@@ -129,7 +133,6 @@ def add_user_dll_extra_paths() -> Optional[List[str]]:
     """
     from configparser import ConfigParser, NoOptionError, NoSectionError
 
-    # os.add_dll_library_path has been added in Python 3.8
     if sys.platform == "win32":
         config_parser = ConfigParser()
         files = config_parser.read(
@@ -148,7 +151,11 @@ def add_user_dll_extra_paths() -> Optional[List[str]]:
         try:
             dll_extra_paths = config_parser.get("Paths", "dll_extra_paths").split(";")
             for path in dll_extra_paths:
-                os.add_dll_directory(path)
+                if path not in _ADDED_DLL_PATHS:
+                    os.add_dll_directory(path)
+                    _ADDED_DLL_PATHS.add(path)
+                else:
+                    logger.debug("Path %r has already been added; skipping" % path)
             return dll_extra_paths
         except (NoOptionError, NoSectionError):
             logger.debug(
@@ -157,10 +164,7 @@ def add_user_dll_extra_paths() -> Optional[List[str]]:
             )
             return None
     else:
-        logger.debug(
-            "Not loading dll_extra_paths because we are not on Windows "
-            "or Python < 3.8"
-        )
+        logger.debug("Not loading dll_extra_paths because we are not on Windows")
         return None
 
 


Test patch:
diff --git a/pyvisa/testsuite/test_util.py b/pyvisa/testsuite/test_util.py
index 3c9015a6..9a1cee33 100644
--- a/pyvisa/testsuite/test_util.py
+++ b/pyvisa/testsuite/test_util.py
@@ -18,6 +18,7 @@
 from typing import Optional
 
 import pytest
+from pytest import LogCaptureFixture, MonkeyPatch
 
 from pyvisa import highlevel, util
 from pyvisa.ctwrapper import IVIVisaLibrary
@@ -45,7 +46,7 @@ def setup_method(self):
             ]
         ):
             raise unittest.SkipTest(
-                ".pyvisarc file exists cannot properly test in this case"
+                ".pyvisarc file exists; cannot properly test in this case"
             )
         self.temp_dir = tempfile.TemporaryDirectory()
         os.makedirs(os.path.join(self.temp_dir.name, "share", "pyvisa"))
@@ -56,6 +57,7 @@ def setup_method(self):
         sys.prefix = self.temp_dir.name
         self._platform = sys.platform
         self._version_info = sys.version_info
+        util._ADDED_DLL_PATHS = set()
 
     def teardown_method(self):
         self.temp_dir.cleanup()
@@ -95,60 +97,137 @@ def test_no_config_file(self, caplog):
 
     # --- Test reading dll_extra_paths.
 
-    def test_reading_config_file_not_windows(self, caplog):
+    def test_reading_config_file_not_windows(self, caplog: LogCaptureFixture):
         sys.platform = "darwin"
+        sys.version_info = (3, 12, 1)  # type: ignore[assignment]
+
         with caplog.at_level(level=logging.DEBUG):
             assert util.add_user_dll_extra_paths() is None
         assert "Not loading dll_extra_paths" in caplog.records[0].message
 
-    def test_reading_config_file_for_dll_extra_paths(self, monkeypatch):
+    def test_reading_config_file_for_dll_extra_paths(self, monkeypatch: MonkeyPatch):
         sys.platform = "win32"
+        extra_paths = [
+            r"C:\Program Files",
+            r"C:\Program Files (x86)",
+        ]
+        added_dll_directories: list[str] = []
         monkeypatch.setattr(
-            os, "add_dll_directory", lambda *args, **kwargs: "", raising=False
+            os,
+            "add_dll_directory",
+            lambda path: added_dll_directories.append(path),
+            raising=False,
         )
         config = ConfigParser()
         config["Paths"] = {}
         config["Paths"]["dll_extra_paths"] = r"C:\Program Files;C:\Program Files (x86)"
+
         with open(self.config_path, "w") as f:
             config.write(f)
-        assert util.add_user_dll_extra_paths() == [
+
+        assert util.add_user_dll_extra_paths() == extra_paths
+        assert added_dll_directories == extra_paths
+
+    def test_reading_config_file_for_dll_extra_paths_multiple_times(
+        self, monkeypatch: MonkeyPatch, caplog: LogCaptureFixture
+    ):
+        sys.platform = "win32"
+        sys.version_info = (3, 8, 1)  # type: ignore[assignment]
+
+        iter_count = 5
+        extra_paths = [
             r"C:\Program Files",
             r"C:\Program Files (x86)",
         ]
+        added_dll_directories: list[str] = []
+        monkeypatch.setattr(
+            os,
+            "add_dll_directory",
+            lambda path: added_dll_directories.append(path),
+            raising=False,
+        )
+        config = ConfigParser()
+        config["Paths"] = {}
+        config["Paths"]["dll_extra_paths"] = r"C:\Program Files;C:\Program Files (x86)"
 
-    def test_no_section_for_dll_extra_paths(self, monkeypatch, caplog):
+        with open(self.config_path, "w") as f:
+            config.write(f)
+
+        with caplog.at_level(level=logging.DEBUG):
+            for _ in range(iter_count):
+                assert util.add_user_dll_extra_paths() == extra_paths
+        skipping_log_messages = [
+            rec.message
+            for rec in caplog.records
+            if "already been added; skipping" in rec.message
+        ]
+        assert added_dll_directories == extra_paths
+        # one log message per path per iteration, except initial add
+        assert (iter_count - 1) * len(extra_paths) == len(skipping_log_messages)
+
+    def test_no_section_for_dll_extra_paths(
+        self, monkeypatch: MonkeyPatch, caplog: LogCaptureFixture
+    ):
         sys.platform = "win32"
+        sys.version_info = (3, 8, 1)  # type: ignore[assignment]
+
+        added_dll_directories: list[str] = []
         monkeypatch.setattr(
-            os, "add_dll_directory", lambda *args, **kwargs: "", raising=False
+            os,
+            "add_dll_directory",
+            lambda path: added_dll_directories.append(path),
+            raising=False,
         )
         config = ConfigParser()
+
         with open(self.config_path, "w") as f:
             config.write(f)
+
         with caplog.at_level(level=logging.DEBUG):
             assert util.add_user_dll_extra_paths() is None
         assert "NoOptionError or NoSectionError" in caplog.records[1].message
+        assert added_dll_directories == []
 
-    def test_no_key_for_dll_extra_paths(self, monkeypatch, caplog):
+    def test_no_key_for_dll_extra_paths(
+        self, monkeypatch: MonkeyPatch, caplog: LogCaptureFixture
+    ):
         sys.platform = "win32"
+        added_dll_directories: list[str] = []
         monkeypatch.setattr(
-            os, "add_dll_directory", lambda *args, **kwargs: "", raising=False
+            os,
+            "add_dll_directory",
+            lambda path: added_dll_directories.append(path),
+            raising=False,
         )
         config = ConfigParser()
         config["Paths"] = {}
+
         with open(self.config_path, "w") as f:
             config.write(f)
+
         with caplog.at_level(level=logging.DEBUG):
             assert util.add_user_dll_extra_paths() is None
         assert "NoOptionError or NoSectionError" in caplog.records[1].message
+        assert added_dll_directories == []
 
-    def test_no_config_file_for_dll_extra_paths(self, monkeypatch, caplog):
+    def test_no_config_file_for_dll_extra_paths(
+        self, monkeypatch: MonkeyPatch, caplog: LogCaptureFixture
+    ):
         sys.platform = "win32"
+        sys.version_info = (3, 8, 1)  # type: ignore[assignment]
+
+        added_dll_directories: list[str] = []
         monkeypatch.setattr(
-            os, "add_dll_directory", lambda *args, **kwargs: "", raising=False
+            os,
+            "add_dll_directory",
+            lambda path: added_dll_directories.append(path),
+            raising=False,
         )
+
         with caplog.at_level(level=logging.DEBUG):
             assert util.add_user_dll_extra_paths() is None
         assert "No user defined" in caplog.records[0].message
+        assert added_dll_directories == []
 
 
 class TestParser(BaseTestCase):