Repository: espressif/pytest-embedded (language: python)

Issue:
pytest-embedded-serial leaves files open after test (RDT-363)
Running tests using pytest-embedded-serial, eventually either OS or python runs out of file handles as the framework seems to leave open file handles after test execution ends. Attached is a example case with three duts, to me running this fails systematically at test 167 and beyond with
> ValueError: filedescriptor out of range in select()

Note: might need to increase OS file handle limit to hit Python's limit above, other option is that tests start to fail with
> OSError: [Errno 24] Too many open files

Environment:
Debian Linux
Python 3.9.2
pytest-embedded 1.2.1
pytest-embedded-serial 1.2.1

[test_open_files.zip](https://github.com/espressif/pytest-embedded/files/10508657/test_open_files.zip)



Discussion / hints:
@JammuKekkonen Thanks for reporting this issue. I can reproduce locally. I'll take a look.

Fix patch:
diff --git a/.github/workflows/build_docs.yml b/.github/workflows/build_docs.yml
index c1986513..b3e2fb49 100644
--- a/.github/workflows/build_docs.yml
+++ b/.github/workflows/build_docs.yml
@@ -7,9 +7,9 @@ jobs:
   test-build-docs:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: Set up Python
-        uses: actions/setup-python@v2
+        uses: actions/setup-python@v4
         with:
           python-version: '3.7'
       - name: Install dependencies
diff --git a/.github/workflows/pre_commit_check.yml b/.github/workflows/pre_commit_check.yml
index c40d535f..46f1356d 100644
--- a/.github/workflows/pre_commit_check.yml
+++ b/.github/workflows/pre_commit_check.yml
@@ -7,8 +7,8 @@ jobs:
   pre-commit:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
         with:
           fetch-depth: 0
-      - uses: actions/setup-python@v2
-      - uses: pre-commit/action@v2.0.2
+      - uses: actions/setup-python@v4
+      - uses: pre-commit/action@v3.0.0
diff --git a/.github/workflows/publish_pypi.yml b/.github/workflows/publish_pypi.yml
index 2aec13e1..c6e04fa0 100644
--- a/.github/workflows/publish_pypi.yml
+++ b/.github/workflows/publish_pypi.yml
@@ -9,9 +9,9 @@ jobs:
   deploy:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: Set up Python
-        uses: actions/setup-python@v2
+        uses: actions/setup-python@v4
         with:
           python-version: '3.7'
       - name: Install dependencies
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 1d75c77f..adf3859f 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -35,7 +35,7 @@ repos:
     hooks:
       - id: flake8
   - repo: https://github.com/pycqa/isort
-    rev: 5.11.4
+    rev: 5.12.0
     hooks:
       - id: isort
         name: isort (python)


Test patch:
diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml
index 061db876..f7610a43 100644
--- a/.github/workflows/build_tests.yml
+++ b/.github/workflows/build_tests.yml
@@ -7,9 +7,9 @@ jobs:
   test-build-packages:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: Set up Python
-        uses: actions/setup-python@v2
+        uses: actions/setup-python@v4
         with:
           python-version: '3.7'
       - name: Install dependencies
diff --git a/.github/workflows/python_tests.yml b/.github/workflows/python_tests.yml
index a154a5dc..b87fbb88 100644
--- a/.github/workflows/python_tests.yml
+++ b/.github/workflows/python_tests.yml
@@ -23,7 +23,7 @@ jobs:
       image: python:${{ matrix.python-version }}-bullseye
       options: --privileged
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: Install dependencies
         run: |
           apt update && apt install -y socat
@@ -32,6 +32,8 @@ jobs:
           pip install cryptography --only-binary :all:
           pip install -r requirements.txt
           bash foreach.sh install
+      - name: Check ports
+        run: ls -la /dev/ttyUSB*
       - name: Run Tests
         if: ${{ matrix.python-version != '3.10' }}
         run: pytest
@@ -49,7 +51,7 @@ jobs:
             --cov=pytest-embedded-qemu/pytest_embedded_qemu/ \
             --cov=pytest-embedded-serial/pytest_embedded_serial/ \
             --cov=pytest-embedded-serial-es/pytest_embedded_serial_esp/
-      - uses: actions/upload-artifact@v2
+      - uses: actions/upload-artifact@v3
         if: failure()
         with:
           name: test_logs
diff --git a/pytest-embedded-serial/pytest_embedded_serial/serial.py b/pytest-embedded-serial/pytest_embedded_serial/serial.py
index 33c5dc59..0929c3a3 100644
--- a/pytest-embedded-serial/pytest_embedded_serial/serial.py
+++ b/pytest-embedded-serial/pytest_embedded_serial/serial.py
@@ -131,7 +131,7 @@ def disable_redirect_thread(self) -> bool:
         kill the redirect thread, and start a new one after got yield back
 
         Yields:
-            True if redirect serial thread is been terminated
+            True if redirect serial thread has been terminated
         """
         killed = self.stop_redirect_thread()
 
diff --git a/pytest-embedded-serial/tests/test_serial.py b/pytest-embedded-serial/tests/test_serial.py
index d26d0e28..b262f84d 100644
--- a/pytest-embedded-serial/tests/test_serial.py
+++ b/pytest-embedded-serial/tests/test_serial.py
@@ -28,3 +28,26 @@ def test_serial_port(dut):
     )
 
     result.assert_outcomes(passed=1)
+
+
+def test_teardown_called_for_multi_dut(testdir):
+    testdir.makepyfile(r"""
+        import pytest
+
+        @pytest.mark.parametrize('count, embedded_services, port', [
+            ('3', 'serial', '/dev/ttyUSB0|/dev/ttyUSB1|/dev/ttyUSB100'),  # set up failure
+        ], indirect=True)
+        def test_teardown_called_for_multi_dut_fail(dut):
+            assert len(dut) == 3
+
+        @pytest.mark.parametrize('count, embedded_services, port', [
+            ('3', 'serial', '/dev/ttyUSB0|/dev/ttyUSB1|/dev/ttyUSB2'),  # set up succeeded
+        ], indirect=True)
+        def test_teardown_called_for_multi_dut_succeeded(dut):
+            assert dut[0].serial.port == '/dev/ttyUSB0'
+            assert dut[1].serial.port == '/dev/ttyUSB1'
+            assert dut[2].serial.port == '/dev/ttyUSB2'
+    """)
+
+    result = testdir.runpytest()
+    result.assert_outcomes(passed=1, errors=1)
diff --git a/pytest-embedded/pytest_embedded/plugin.py b/pytest-embedded/pytest_embedded/plugin.py
index 24077fde..8809e7c9 100644
--- a/pytest-embedded/pytest_embedded/plugin.py
+++ b/pytest-embedded/pytest_embedded/plugin.py
@@ -393,34 +393,34 @@ def multi_dut_generator_fixture(
     @functools.wraps(func)
     def wrapper(*args, **kwargs):
         def _close_or_terminate(obj):
+            if obj is None:
+                del obj
+                return
+
             try:
-                if isinstance(obj, subprocess.Popen):
-                    obj.terminate()
-                    obj.kill()
-                elif isinstance(obj, multiprocessing.Process):
+                if isinstance(obj, (subprocess.Popen, multiprocessing.process.BaseProcess)):
                     obj.terminate()
                     obj.kill()
                 elif isinstance(obj, io.IOBase):
                     try:
                         obj.close()
                     except Exception as e:
-                        logging.debug('file close failed')
-                        logging.debug(str(e))
-                        raise
+                        logging.debug('file %s closed failed with error: %s', obj, str(e))
+                else:
+                    try:
+                        obj.close()
+                    except AttributeError:
+                        try:
+                            obj.terminate()
+                        except AttributeError:
+                            pass
+                    except Exception as e:
+                        logging.debug('Not properly caught object %s: %s', obj, str(e))
             except Exception as e:
                 logging.debug('%s: %s', obj, str(e))
                 return  # swallow up all error
-
-            try:
-                obj.close()
-            except AttributeError:
-                try:
-                    obj.terminate()
-                except AttributeError:
-                    pass
-            except Exception as e:
-                logging.debug('Not properly caught object %s: %s', obj, str(e))
-                return  # swallow up all error
+            finally:
+                del obj
 
         if _COUNT == 1:
             res = None
@@ -441,7 +441,15 @@ def _close_or_terminate(obj):
                     else:
                         current_kwargs[k] = v
 
-                res.append(func(*args, **current_kwargs))
+                try:
+                    i_res = func(*args, **current_kwargs)
+                    res.append(i_res)
+                except Exception:  # noqa
+                    for item in res:  # close the earlier succeeded set up items
+                        _close_or_terminate(item)
+
+                    raise
+
             try:
                 yield res
             finally:
@@ -623,9 +631,7 @@ def _listen(q: MessageQueue, filepath: str, with_timestamp: bool = True, count:
 
 
 @pytest.fixture
-# here we use @multi_dut_fixture
-# The daemon process should be closed at the very last. would be auto closed.
-@multi_dut_fixture
+@multi_dut_generator_fixture
 def _listener(msg_queue, _pexpect_logfile, with_timestamp, dut_index, dut_total) -> multiprocessing.Process:
     """
     The listener would create a `_listen` process. The `_listen` process would get the string from the message queue,
@@ -648,7 +654,6 @@ def _listener(msg_queue, _pexpect_logfile, with_timestamp, dut_index, dut_total)
             _pexpect_logfile,
         ),
         kwargs=_drop_none_kwargs(kwargs),
-        daemon=True,
     )
 
 
diff --git a/pytest-embedded/tests/test_base.py b/pytest-embedded/tests/test_base.py
index 418de13e..cf60b412 100644
--- a/pytest-embedded/tests/test_base.py
+++ b/pytest-embedded/tests/test_base.py
@@ -551,3 +551,28 @@ def test_quick_example(redirect, dut: Dut):
     """)
     result = testdir.runpytest('--root-logdir', testdir.tmpdir)
     result.assert_outcomes(passed=1)
+
+
+def test_unclosed_file_handler(testdir):
+    """
+    select only support fd < FD_SETSIZE (1024)
+
+    Related Links:
+        - https://github.com/python/cpython/blob/v3.11.1/Include/fileobject.h#L37
+        - https://man7.org/linux/man-pages/man2/select.2.html
+    """
+    testdir.makepyfile(r"""
+    import pytest
+
+    @pytest.mark.parametrize("test_input", range(0, 1024))
+    def test_unclosed_file_handler(test_input, dut):
+        dut[0].write("foo")
+        assert test_input == test_input
+    """)
+    result = testdir.runpytest(
+        '--embedded-services', 'serial',
+        '--count', '3',
+        '--port', '/dev/ttyUSB0|/dev/ttyUSB1|/dev/ttyUSB2',
+        '-x',  # fail at the first fail
+    )
+    result.assert_outcomes(passed=1024)