Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tmpdir_factory deletes tmp dirs from other pytest processes after 3 hours, even if they are still running #7911

Closed
danc86 opened this issue Oct 19, 2020 · 10 comments

Comments

@danc86
Copy link

@danc86 danc86 commented Oct 19, 2020

We use pytest in a Jenkins job, which ends running many concurrent pytest processes on a single Linux worker under the same user account (jenkins). The test suite has some session-scoped fixtures which use tmpdir_factory to create temporary directories and create files in them, which are then used throughout the duration of the entire test run.

Recently our pytest runs started taking longer than 3 hours and we started to see tests randomly fail near the end of the suite with errors like this:

FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-jenkins/pytest-47927/...

indicating that something had deleted the tmp files created by the session-scoped fixture at the beginning of the test run.

Pytest's tmpdir_factory fixture calls make_numbered_dir_with_cleanup to handle creation and cleanup of tmp directories:
https://github.com/pytest-dev/pytest/blob/4.4.0/src/_pytest/tmpdir.py#L75
That function has some logic which looks for the presence of a lock file to detect if the directory is still being used by another pytest process:
https://github.com/pytest-dev/pytest/blob/4.4.0/src/_pytest/pathlib.py#L205
but the logic assumes that any lock file older than the given "lock timeout" (hardcoded to 3 hours) is stale and can just be deleted. But if the other pytest process is actually still running after 3 hours, its tmp directory will be deleted while it is still in use.

The Linux worker is CentOS 7.8 with Python 3.6.8 and pytest 4.4.0. I see the pytest master branch still has the same 3 hour timeout logic so I think this bug will affect all recent versions of pytest.

@danc86
Copy link
Author

@danc86 danc86 commented Oct 19, 2020

Rather than checking modtimes, it would be nicer to use an actual advisory lock (as in flock). If the lock is still held by another process then you know it's alive, regardless how old the lock file is. If it's not held, then the other process died and the directory is stale and can be cleaned up. No need to pick a hardcoded "stale" timeout.

I could post a PR for that. However I am not sure how to make that portable to non-UNIX platforms. I don't use Windows.

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 19, 2020

I recall the timeout being much larger (either 24h or 3d)

Threading this as regression for now

@danc86
Copy link
Author

@danc86 danc86 commented Oct 19, 2020

It's hardcoded as 60 * 60 * 3 seconds = 3 hours on master (and has been unchanged for quite a few versions, according to the git history):

https://github.com/pytest-dev/pytest/blob/master/src/_pytest/pathlib.py#L35

@Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Oct 19, 2020

Using flock can cause trouble if test sessions die without releasing the lock via something like an interpreter segfault.

Personally I'd be happy to just increase the timeout to three days, as a simple solution. (we could also make LOCK_TIMEOUT a default argument value instead of a global, since it's only ever used in tmpdir)

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 19, 2020

d76fa59 introduced it,

i believe it is a argument and the global is the default
pylib originally had https://github.com/pytest-dev/py/blob/d4111cae5572809a23d3d5cc88bed8e4af720929/py/_path/local.py#L840-L841 - so i believe we should add it again

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Oct 19, 2020

Personally I'd be happy to just increase the timeout to three days, as a simple solution.

I agree increasing to a few days is simpler and probably good enough.

vgerak added a commit to vgerak/pytest that referenced this issue Oct 19, 2020
vgerak added a commit to vgerak/pytest that referenced this issue Oct 19, 2020
@danc86
Copy link
Author

@danc86 danc86 commented Oct 19, 2020

A flock is associated with an open file descriptor so it is always released whenever the process holding the flock exits (whether cleanly or abnormally, due to a signal, etc), the OS guarantees this. It can't be accidentally leaked. So flock would be the right mechanism to use for this, on UNIX at least.

@danc86
Copy link
Author

@danc86 danc86 commented Oct 28, 2020

@vgerak are you planning to file a PR to increase the timeout to 3 days? Or would you like me to?

@vgerak
Copy link
Contributor

@vgerak vgerak commented Oct 28, 2020

@vgerak are you planning to file a PR to increase the timeout to 3 days? Or would you like me to?

@danc86 There has been one (that has been approved, but not yet merged). Apparently I hadn't linked it to the issue (only mentioned it on the commits), thanks for the heads-up!

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Oct 28, 2020

Sorry for the delay, thanks everyone!

bors bot added a commit to aragilar/pytest-info-collector that referenced this issue Mar 29, 2021
Merge #14 #17
14: Update sphinx_rtd_theme to 0.5.1 r=aragilar a=pyup-bot


This PR updates [sphinx_rtd_theme](https://pypi.org/project/sphinx_rtd_theme) from **0.5.0** to **0.5.1**.



*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/sphinx-rtd-theme
  - Repo: https://github.com/readthedocs/sphinx_rtd_theme
</details>



17: Update pytest to 6.2.2 r=aragilar a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **6.1.1** to **6.2.2**.



<details>
  <summary>Changelog</summary>
  
  
   ### 6.2.2
   ```
   =========================

Bug Fixes
---------

- `8152 &lt;https://github.com/pytest-dev/pytest/issues/8152&gt;`_: Fixed &quot;(&lt;Skipped instance&gt;)&quot; being shown as a skip reason in the verbose test summary line when the reason is empty.


- `8249 &lt;https://github.com/pytest-dev/pytest/issues/8249&gt;`_: Fix the ``faulthandler`` plugin for occasions when running with ``twisted.logger`` and using ``pytest --capture=no``.
   ```
   
  
  
   ### 6.2.1
   ```
   =========================

Bug Fixes
---------

- `7678 &lt;https://github.com/pytest-dev/pytest/issues/7678&gt;`_: Fixed bug where ``ImportPathMismatchError`` would be raised for files compiled in
  the host and loaded later from an UNC mounted path (Windows).


- `8132 &lt;https://github.com/pytest-dev/pytest/issues/8132&gt;`_: Fixed regression in ``approx``: in 6.2.0 ``approx`` no longer raises
  ``TypeError`` when dealing with non-numeric types, falling back to normal comparison.
  Before 6.2.0, array types like tf.DeviceArray fell through to the scalar case,
  and happened to compare correctly to a scalar if they had only one element.
  After 6.2.0, these types began failing, because they inherited neither from
  standard Python number hierarchy nor from ``numpy.ndarray``.

  ``approx`` now converts arguments to ``numpy.ndarray`` if they expose the array
  protocol and are not scalars. This treats array-like objects like numpy arrays,
  regardless of size.
   ```
   
  
  
   ### 6.2.0
   ```
   =========================

Breaking Changes
----------------

- `7808 &lt;https://github.com/pytest-dev/pytest/issues/7808&gt;`_: pytest now supports python3.6+ only.



Deprecations
------------

- `7469 &lt;https://github.com/pytest-dev/pytest/issues/7469&gt;`_: Directly constructing/calling the following classes/functions is now deprecated:

  - ``_pytest.cacheprovider.Cache``
  - ``_pytest.cacheprovider.Cache.for_config()``
  - ``_pytest.cacheprovider.Cache.clear_cache()``
  - ``_pytest.cacheprovider.Cache.cache_dir_from_config()``
  - ``_pytest.capture.CaptureFixture``
  - ``_pytest.fixtures.FixtureRequest``
  - ``_pytest.fixtures.SubRequest``
  - ``_pytest.logging.LogCaptureFixture``
  - ``_pytest.pytester.Pytester``
  - ``_pytest.pytester.Testdir``
  - ``_pytest.recwarn.WarningsRecorder``
  - ``_pytest.recwarn.WarningsChecker``
  - ``_pytest.tmpdir.TempPathFactory``
  - ``_pytest.tmpdir.TempdirFactory``

  These have always been considered private, but now issue a deprecation warning, which may become a hard error in pytest 7.0.0.


- `7530 &lt;https://github.com/pytest-dev/pytest/issues/7530&gt;`_: The ``--strict`` command-line option has been deprecated, use ``--strict-markers`` instead.

  We have plans to maybe in the future to reintroduce ``--strict`` and make it an encompassing flag for all strictness
  related options (``--strict-markers`` and ``--strict-config`` at the moment, more might be introduced in the future).


- `7988 &lt;https://github.com/pytest-dev/pytest/issues/7988&gt;`_: The ``pytest.yield_fixture`` decorator/function is now deprecated. Use :func:`pytest.fixture` instead.

  ``yield_fixture`` has been an alias for ``fixture`` for a very long time, so can be search/replaced safely.



Features
--------

- `5299 &lt;https://github.com/pytest-dev/pytest/issues/5299&gt;`_: pytest now warns about unraisable exceptions and unhandled thread exceptions that occur in tests on Python&gt;=3.8.
  See :ref:`unraisable` for more information.


- `7425 &lt;https://github.com/pytest-dev/pytest/issues/7425&gt;`_: New :fixture:`pytester` fixture, which is identical to :fixture:`testdir` but its methods return :class:`pathlib.Path` when appropriate instead of ``py.path.local``.

  This is part of the movement to use :class:`pathlib.Path` objects internally, in order to remove the dependency to ``py`` in the future.

  Internally, the old :class:`Testdir &lt;_pytest.pytester.Testdir&gt;` is now a thin wrapper around :class:`Pytester &lt;_pytest.pytester.Pytester&gt;`, preserving the old interface.


- `7695 &lt;https://github.com/pytest-dev/pytest/issues/7695&gt;`_: A new hook was added, `pytest_markeval_namespace` which should return a dictionary.
  This dictionary will be used to augment the &quot;global&quot; variables available to evaluate skipif/xfail/xpass markers.

  Pseudo example

  ``conftest.py``:

  .. code-block:: python

     def pytest_markeval_namespace():
         return {&quot;color&quot;: &quot;red&quot;}

  ``test_func.py``:

  .. code-block:: python

     pytest.mark.skipif(&quot;color == &#39;blue&#39;&quot;, reason=&quot;Color is not red&quot;)
     def test_func():
         assert False


- `8006 &lt;https://github.com/pytest-dev/pytest/issues/8006&gt;`_: It is now possible to construct a :class:`~pytest.MonkeyPatch` object directly as ``pytest.MonkeyPatch()``,
  in cases when the :fixture:`monkeypatch` fixture cannot be used. Previously some users imported it
  from the private `_pytest.monkeypatch.MonkeyPatch` namespace.

  Additionally, :meth:`MonkeyPatch.context &lt;pytest.MonkeyPatch.context&gt;` is now a classmethod,
  and can be used as ``with MonkeyPatch.context() as mp: ...``. This is the recommended way to use
  ``MonkeyPatch`` directly, since unlike the ``monkeypatch`` fixture, an instance created directly
  is not ``undo()``-ed automatically.



Improvements
------------

- `1265 &lt;https://github.com/pytest-dev/pytest/issues/1265&gt;`_: Added an ``__str__`` implementation to the :class:`~pytest.pytester.LineMatcher` class which is returned from ``pytester.run_pytest().stdout`` and similar. It returns the entire output, like the existing ``str()`` method.


- `2044 &lt;https://github.com/pytest-dev/pytest/issues/2044&gt;`_: Verbose mode now shows the reason that a test was skipped in the test&#39;s terminal line after the &quot;SKIPPED&quot;, &quot;XFAIL&quot; or &quot;XPASS&quot;.


- `7469 &lt;https://github.com/pytest-dev/pytest/issues/7469&gt;`_ The types of builtin pytest fixtures are now exported so they may be used in type annotations of test functions.
  The newly-exported types are:

  - ``pytest.FixtureRequest`` for the :fixture:`request` fixture.
  - ``pytest.Cache`` for the :fixture:`cache` fixture.
  - ``pytest.CaptureFixture[str]`` for the :fixture:`capfd` and :fixture:`capsys` fixtures.
  - ``pytest.CaptureFixture[bytes]`` for the :fixture:`capfdbinary` and :fixture:`capsysbinary` fixtures.
  - ``pytest.LogCaptureFixture`` for the :fixture:`caplog` fixture.
  - ``pytest.Pytester`` for the :fixture:`pytester` fixture.
  - ``pytest.Testdir`` for the :fixture:`testdir` fixture.
  - ``pytest.TempdirFactory`` for the :fixture:`tmpdir_factory` fixture.
  - ``pytest.TempPathFactory`` for the :fixture:`tmp_path_factory` fixture.
  - ``pytest.MonkeyPatch`` for the :fixture:`monkeypatch` fixture.
  - ``pytest.WarningsRecorder`` for the :fixture:`recwarn` fixture.

  Constructing them is not supported (except for `MonkeyPatch`); they are only meant for use in type annotations.
  Doing so will emit a deprecation warning, and may become a hard-error in pytest 7.0.

  Subclassing them is also not supported. This is not currently enforced at runtime, but is detected by type-checkers such as mypy.


- `7527 &lt;https://github.com/pytest-dev/pytest/issues/7527&gt;`_: When a comparison between :func:`namedtuple &lt;collections.namedtuple&gt;` instances of the same type fails, pytest now shows the differing field names (possibly nested) instead of their indexes.


- `7615 &lt;https://github.com/pytest-dev/pytest/issues/7615&gt;`_: :meth:`Node.warn &lt;_pytest.nodes.Node.warn&gt;` now permits any subclass of :class:`Warning`, not just :class:`PytestWarning &lt;pytest.PytestWarning&gt;`.


- `7701 &lt;https://github.com/pytest-dev/pytest/issues/7701&gt;`_: Improved reporting when using ``--collected-only``. It will now show the number of collected tests in the summary stats.


- `7710 &lt;https://github.com/pytest-dev/pytest/issues/7710&gt;`_: Use strict equality comparison for non-numeric types in :func:`pytest.approx` instead of
  raising :class:`TypeError`.

  This was the undocumented behavior before 3.7, but is now officially a supported feature.


- `7938 &lt;https://github.com/pytest-dev/pytest/issues/7938&gt;`_: New ``--sw-skip`` argument which is a shorthand for ``--stepwise-skip``.


- `8023 &lt;https://github.com/pytest-dev/pytest/issues/8023&gt;`_: Added ``&#39;node_modules&#39;`` to default value for :confval:`norecursedirs`.


- `8032 &lt;https://github.com/pytest-dev/pytest/issues/8032&gt;`_: :meth:`doClassCleanups &lt;unittest.TestCase.doClassCleanups&gt;` (introduced in :mod:`unittest` in Python and 3.8) is now called appropriately.



Bug Fixes
---------

- `4824 &lt;https://github.com/pytest-dev/pytest/issues/4824&gt;`_: Fixed quadratic behavior and improved performance of collection of items using autouse fixtures and xunit fixtures.


- `7758 &lt;https://github.com/pytest-dev/pytest/issues/7758&gt;`_: Fixed an issue where some files in packages are getting lost from ``--lf`` even though they contain tests that failed. Regressed in pytest 5.4.0.


- `7911 &lt;https://github.com/pytest-dev/pytest/issues/7911&gt;`_: Directories created by by :fixture:`tmp_path` and :fixture:`tmpdir` are now considered stale after 3 days without modification (previous value was 3 hours) to avoid deleting directories still in use in long running test suites.


- `7913 &lt;https://github.com/pytest-dev/pytest/issues/7913&gt;`_: Fixed a crash or hang in :meth:`pytester.spawn &lt;_pytest.pytester.Pytester.spawn&gt;` when the :mod:`readline` module is involved.


- `7951 &lt;https://github.com/pytest-dev/pytest/issues/7951&gt;`_: Fixed handling of recursive symlinks when collecting tests.


- `7981 &lt;https://github.com/pytest-dev/pytest/issues/7981&gt;`_: Fixed symlinked directories not being followed during collection. Regressed in pytest 6.1.0.


- `8016 &lt;https://github.com/pytest-dev/pytest/issues/8016&gt;`_: Fixed only one doctest being collected when using ``pytest --doctest-modules path/to/an/__init__.py``.



Improved Documentation
----------------------

- `7429 &lt;https://github.com/pytest-dev/pytest/issues/7429&gt;`_: Add more information and use cases about skipping doctests.


- `7780 &lt;https://github.com/pytest-dev/pytest/issues/7780&gt;`_: Classes which should not be inherited from are now marked ``final class`` in the API reference.


- `7872 &lt;https://github.com/pytest-dev/pytest/issues/7872&gt;`_: ``_pytest.config.argparsing.Parser.addini()`` accepts explicit ``None`` and ``&quot;string&quot;``.


- `7878 &lt;https://github.com/pytest-dev/pytest/issues/7878&gt;`_: In pull request section, ask to commit after editing changelog and authors file.



Trivial/Internal Changes
------------------------

- `7802 &lt;https://github.com/pytest-dev/pytest/issues/7802&gt;`_: The ``attrs`` dependency requirement is now &gt;=19.2.0 instead of &gt;=17.4.0.


- `8014 &lt;https://github.com/pytest-dev/pytest/issues/8014&gt;`_: `.pyc` files created by pytest&#39;s assertion rewriting now conform to the newer PEP-552 format on Python&gt;=3.7.
  (These files are internal and only interpreted by pytest itself.)
   ```
   
  
  
   ### 6.1.2
   ```
   =========================

Bug Fixes
---------

- `7758 &lt;https://github.com/pytest-dev/pytest/issues/7758&gt;`_: Fixed an issue where some files in packages are getting lost from ``--lf`` even though they contain tests that failed. Regressed in pytest 5.4.0.


- `7911 &lt;https://github.com/pytest-dev/pytest/issues/7911&gt;`_: Directories created by `tmpdir` are now considered stale after 3 days without modification (previous value was 3 hours) to avoid deleting directories still in use in long running test suites.



Improved Documentation
----------------------

- `7815 &lt;https://github.com/pytest-dev/pytest/issues/7815&gt;`_: Improve deprecation warning message for ``pytest._fillfuncargs()``.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants