Cleaning up tmpdir's

[The-Compiler]

Currently it seems pytest never cleans up the temporary directories it creates. Is this intended?

I can see how it can be useful when tests fail and one wants to inspect the contents of the temporary folder - but it can also cause a lot of trouble with long-running systems (I had the harddisk of a VM fill up because >2 GB of pytest tempfiles accumulated) or systems not cleaning the tempdir (Windows, I'm looking at you!).

I can think of some solutions to this:

• Add a new argument and always clean up tempdirs after a test unless that argument is given.
• Only clean up tempdirs for passing tests, and keep them for failing tests
• Clean up tempdirs after x runs (when a new run is started), or after they reached a certain age treshold.

[RonnyPfannschmidt]

the default was to clean up all but the last 3 folders? is this a regression?

[The-Compiler]

From a cursory look I can't find anything related to cleanup in the code, even in the history... f456e37 from 2010 adds a TempdirHandler.finish method, but that only does self.trace("finish").

[RonnyPfannschmidt]

py.path.local.make_numbered_dir has the cleanup code

[nicoddemus]

Any news on this?

[RonnyPfannschmidt]

didnt look into it

[The-Compiler]

Me neither, but I still have some pytest dirs lying around on my CI VMs:

pytest-0
pytest-1
pytest-10
pytest-11
pytest-12
pytest-13
pytest-14
pytest-15
pytest-16
pytest-17
pytest-18
pytest-19
pytest-2
pytest-3
pytest-4
pytest-5
pytest-6
pytest-7
pytest-8
pytest-9
pytest-buildbot

pytest-0
pytest-1
pytest-2
pytest-3
pytest-4
pytest-5
pytest-6
pytest-7
pytest-8
pytest-9
pytest-buildbotx

pytest-0
pytest-1
pytest-10
pytest-2
pytest-3
pytest-4
pytest-5
pytest-6
pytest-7
pytest-8
pytest-9
pytest-buildbotx

pytest-0
pytest-1
pytest-2
pytest-3
pytest-4
pytest-5
pytest-6
pytest-7
pytest-8
pytest-9
pytest-buildbotx

(ls /tmp/buildbot-of-* on three VMs)

[nicoddemus]

Weird.

I think the code which removes old directories might be silently eating some exception:

# contents of py._path.local:847
        if keep:
            for path in rootdir.listdir():
                num = parse_num(path)
                if num is not None and num <= (maxnum - keep):
                    lf = path.join('.lock')
                    try:
                        t1 = lf.lstat().mtime
                        t2 = lockfile.lstat().mtime
                        if not lock_timeout or abs(t2-t1) < lock_timeout:
                            continue   # skip directories still locked
                    except py.error.Error:
                        pass   # assume that it means that there is no 'lf'
                    try:
                        path.remove(rec=1)
                    except KeyboardInterrupt:
                        raise
                    except: # this might be py.error.Error, WindowsError ...
                        pass

[The-Compiler]

Either way, except: pass is a really bad idea.

[nicoddemus]

Agreed

[obestwalter]

This looks like low hanging fruit for new contributors.

[rjw245]

I'm a new contributor :) Happy to take a look at this.

[nicoddemus]

Appreciated! Let us know if you need help 👍

[rjw245]

I think it's a concurrency problem.
I can cause something similar to happen when I spawn many instances of pytest on the same test suite

pytest & pytest & pytest & pytest & pytest & pytest & pytest & pytest & 

Now in my pytest-of-*/ directory I'll see more than 4 directories left over (I typically see the most recent 4 -- not sure if this should be 3 instead):

pytest-49  pytest-51  pytest-53  pytest-55  pytest-57
pytest-50  pytest-52  pytest-54  pytest-56  pytest-riley

I suspect this could happen any time multiple tests that use temporary directories are run at the same time -- doesn't have to be the same test suite. @The-Compiler Do your CI VMs run many tests that use temporary directories at once?

I've changed the except: pass that @nicoddemus found in the py library to except: raise and now I sporadically get the following exception when spawning many pytest instances at once:

ENOENT: [No such file or directory]: rmtree('/tmp/pytest-of-riley/pytest-70',)

So you were right @nicoddemus, there's an exception being eaten.
This exception will crop up even with just two instances of pytest running concurrently (though I won't see extra pytest-N folders in my base temp directory). What I do see is the pytest-N numbering jumping by one more than I would expect.

Contents of pytest-of-riley/ before running tests:

pytest-30     pytest-31     pytest-32     pytest-33   pytest-riley

I run two instances concurrently:

pytest & pytest &

And when the stars align, I get this exception:

ENOENT: [No such file or directory]: remove('/tmp/pytest-of-riley/pytest-30',)

And now the contents of pytest-of-riley/ are:

pytest-33     pytest-34     pytest-35     pytest-36    pytest-riley

I'm not sure where pytest-36 came from. Also, pytest-35 is completely empty (meaning only two of the new folders were used, as you'd expect). This is repeatable -- the second-to-maximum temp dir is always unused when encountering this exception with two concurrent pytest instances.

I think this is some good information, but I'm still not sure where this behavior comes from. Perhaps the lockfile in make_numbered_dir needs to be acquired earlier (when it's calculating the maximum dir number)?
Any ideas?
Side note: should probably add a test which ensures correct behavior when working with temp dirs concurrently.

[nicoddemus]

Any ideas?

Not at the moment, but you should try your suggestion of acquiring the lock file early and see how that goes. 👍

[nvgoldin]

Noticing the same issue here.

[RonnyPfannschmidt]

i did some more reading - there wil lbe a lockfile which will be closed at normal exit

in case py.test fails to remove it, the lock is ignored after 2 days

we wont delete folders belonging to running processes, and we wont delete folders of failed processes

[avirlrma]

This looks doable,I would like to take this on @RonnyPfannschmidt @nicoddemus @rjw245

[avirlrma]

Can I start working on this? @RonnyPfannschmidt @nicoddemus

[nicoddemus]

@avirlrma sure, please go ahead! 😁

[avirlrma]

Thanks! @nicoddemus

[avirlrma]

@nicoddemus
I tried to re-create what @rjw245 was doing i.e.:
Ran multiple instances of pytest using
pytest & pytest &
on a test like this

def test_create_file(tmpdir):
    p = tmpdir.mkdir("sub").join("hello.txt")
    p.write("content")
    assert p.read() == "content"
    assert len(tmpdir.listdir()) == 1
    assert 0

But in my pytest-of-* I have only 4 folders. What am I doing wrong?

[RonnyPfannschmidt]

starting something like 20 instances at once should demonstrate the issue, also a sleep to make things slow helps

[RonnyPfannschmidt]

@istepanko at first glance its not clear why pytest-parallel inhibits the tmpdir cleanup - but boy is that one a hack

[thomashoang]

Hello, from my understanding of the issue, pytest is creating many temporary files from being run and it is taking up too much space. Has anyone tried to create a script that would run when given a flag on the command line that would delete all temporary files every time? If not I would like to attempt this approach. For reference I am a student at the University of Michigan and I have to contribute to an open source project for a class.

[atugushev]

I've noticed that pytest-xdist works like a charm. But if you run pytest simultaneously N-times or use pytest-parallel qty of temporary dirs will increase.

[RonnyPfannschmidt]

pytest-xdist ensures the correct basetemp setup for worker processes,

pytest-parallel does not, and on top of that does very interesting things to pytest to "create" thread-safety

[atugushev]

FTR cleanup_on_exit will never be called with pytest-parallel, since it uses multiprocessing, which does not call registered functions via atexit. That's why there are dead locks after each worker being run.

pytest/src/_pytest/pathlib.py

Lines 163 to 177 in dc75b6a

	def register_cleanup_lock_removal(lock_path, register=atexit.register):
	"""registers a cleanup function for removing a lock, by default on atexit"""
	pid = os.getpid()
	def cleanup_on_exit(lock_path=lock_path, original_pid=pid):
	current_pid = os.getpid()
	if current_pid != original_pid:
	# fork
	return
	try:
	lock_path.unlink()
	except (OSError, IOError):
	pass
	return register(cleanup_on_exit)

The multiprocessing under the hood calls (see py27 and py37 implementation) os._exit at the end of the each processes, that's why atexit is not called, see a note from atexit doc:

Note: The functions registered via this module are not called when the program is killed by a signal not handled by Python, when a Python fatal internal error is detected, or when os._exit() is called.

[RonnyPfannschmidt]

one of the reasons why xdist takes over the tmpdir handing for workers

[bitmuster]

Hi all,

I've investigated in this issue during the europython sprints.
I seems like temporary directories are cleaned up when a new one is created.
While they are in use they are locked. The locks are removed by help of an atexit handler.
Hence, locks will get always removed but not the temporary directories when lots of pytests run in parallel.

I moved directory cleanup also to an atexit handler that is run after the lock cleanup.
Do you think this is a feasible solution?

The fix is available here (pullrequest is prepared):
https://github.com/bitmuster/pytest/tree/issue_1120

After the fix I do not see leftover directories (apart from the three that are always kept).
I do not have an automated test for this yet. Here is how I tested it (Linux & Python 3.7.4):

test_tempdir.py

def test_create_file(tmpdir):
    p = tmpdir.mkdir("sub").join("hello.txt")
    p.write("content")
    assert p.read() == "content"
    assert len(tmpdir.listdir()) == 1

def test_create_file2(tmpdir):
    p = tmpdir.mkdir("sub2").join("hello2.txt")
    p.write("content2")
    assert p.read() == "content2"
    assert len(tmpdir.listdir()) == 1

def test_create_file3(tmpdir):
    p = tmpdir.mkdir("sub3").join("hello3.txt")
    p.write("content3")
    assert p.read() == "content3"
    assert len(tmpdir.listdir()) == 1

Execute it 100 times in parallel:

for i in $(seq 1 100); do pytest -s & done

Note: This is not yet tested with pytest-parallel (Comment from atugushev)

[RonnyPfannschmidt]

pytest intentionally leaves the tmpdirs around for later removal, this is to support debugging,

if stuf fbreaks and the tmpdir is already deleted, one has a hard time to post-mortem

[The-Compiler]

@RonnyPfannschmidt Did you read @bitmuster's comment, or take a look at the fix at all?

After the fix I do not see leftover directories (apart from the three that are always kept).

[RonnyPfannschmidt]

i did misread it it seems, i certainly stand corrected