pipenv install shows a ResourceWarning if installing fails

[Julian]

(This is obviously minor, since the process is likely to exit anyhow when this happens, but filing for posterity)

My pipenv install is failing (for reasons I can't decipher yet, so ignore the traceback), but during the failure it looks like there's also a ResourceWarning being shown

⊙  pipenv install                                                                                                                                                                                                                                                                                                                                         Julian@Macnetic ●
Warning: Your Pipfile requires python_version pypy, but you are using 2.7.13 (/Users/Julian/L/A/v/M/bin/python).
  $ pipenv check will surely fail.
Installing dependencies from Pipfile.lock (ee6476)…
Traceback (most recent call last):▉▉▉▉▉ 0/65 — 00:00:00
  File "/Users/Julian/.local/bin/pipenv", line 11, in <module>
    sys.exit(cli())
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/cli.py", line 195, in install
    selective_upgrade=selective_upgrade
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/core.py", line 1854, in do_install
    pre=pre, requirements_dir=requirements_directory
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/core.py", line 1394, in do_init
    requirements_dir=requirements_dir.name)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/core.py", line 877, in do_install_dependencies
    requirements_dir=requirements_dir
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/core.py", line 1495, in pip_install
    c = delegator.run(pip_command, block=block)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/delegator.py", line 267, in run
    c.run(block=block, binary=binary)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/delegator.py", line 156, in run
    s = PopenSpawn(self._popen_args, **pexpect_kwargs)
  File "/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/vendor/pexpect/popen_spawn.py", line 46, in __init__
    self.proc = subprocess.Popen(cmd, **kwargs)
  File "/usr/local/Cellar/pypy/5.10.0_1/libexec/lib-python/2.7/subprocess.py", line 405, in __init__
    errread, errwrite)
  File "/usr/local/Cellar/pypy/5.10.0_1/libexec/lib-python/2.7/subprocess.py", line 1053, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
/Users/Julian/.local/share/virtualenvs/pipenv/site-packages/pipenv/utils.py:1157: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/var/folders/kh/mscfrt910qb_qb8wqrz7t_7w0000gn/T/pipenv-oH3l7R-requirements'>
  warnings.warn(warn_message, ResourceWarning)

which appears to be because pipenv.core.do_init creates a TemporaryDirectory without using a context manager or contextlib.ExitStack.

[uranusjr]

Contributions are welcomed!

[techalchemy]

@Julian the inference you're making is mostly right, but that resource is created out of necessity... one interesting point is that I backported TemporaryDirectory + weakref.finalize for python2.7 support, and I have no idea how that works with pypy.

We can't handle this with a context manager because the resource is actually passed around through several function calls and then ultimately to a subprocess to handle resolution. You're actually hitting a (fixed, I believe) bug in the resolver stack where the resolver sometimes calls TemporaryDirectory.cleanup() on temporary directories it shouldn't. Can you check with master and see if you still experience this?

[Julian]

@techalchemy you can still explicitly clean up the resource though, by using ExitStack, and doing the function calls and subprocess firing within the ExitStack. (And this isn't strictly PyPy related, it's the same as if you were say opening files without using with, where yeah on CPython it doesn't show noticeable negative effects but is "frowned upon" stylistically anyhow).

I'll check on master though.

[uranusjr]

If you decide to work on this, remember contextlib.ExitStack does not exist in Python 2 :)

[Julian]

It's backported in contextlib2.

[techalchemy]

@Julian but I am telling you that we do explicitly clean up the resource. Not all of the things you described apply here -- we spawn a bunch of subprocesses which own their own process groups and they all write concurrently to the same TemporaryDirectory. There is no option to use context managers or pass context around.

[techalchemy]

At least not that I am aware of, but if you know of a way to handle this I would certainly be interested. I'd say to look at the exact circumstances to make sure it applies before assuming it does though, I have spent a bunch of time tracking down these types of issues due to maintaining cross platform compatibility with windows.

[Julian]

The warning only happens if this was in fact not cleaned up, I looked at the code afterwards to confirm. It's being explicitly cleaned up, but not in a context manager, so an error (like the one that's happening to me here which is #2321) means that requirements_dir.cleanup never runs.

E.g., on this branch: https://github.com/pypa/pipenv/blob/master/pipenv/core.py#L1294-L1313 if an exception is thrown anywhere above the .cleanup line, the directory wasn't cleaned up.

The way to fix is to make everything from 1294 to 1376 be inside a with block (regardless of other functions being called or subprocesses using the same resource) [but since you only conditionally create the tempdir, you'd need ExitStack].

I'm not sure I'll get a chance to actually implement a fix myself, but yes I'm sure it'd be easier to explain if I did that :P. Will see what I can do.

Also I'm not concentrating hard enough and out of all the issues this one is definitely the least important :P, so probably not worth discussing too much if I'm not being clear (or if I'm straight up wrong).

[techalchemy]

That's actually super clear, I didn't spend a lot of time on this because it's handled implicitly (I considered disabling the warning since the weakref.finalize call handles it). I'm actually not convinced this would help on Windows though, because forked subprocess trees spawn an additional layer of subprocesses if there is a VCS install and windows never gives back permissions on vcs indexes and also leaks open file handles during these calls. If the solution you describe can adequately handle this case, it would be awesome, I've actually invested a lot of effort trying to clean up the process trees and fix the permissions and that obviously doesn't work super well.

[techalchemy]

@ncoghlan I'd love to get your take on this -- at a high level, does a direct ExitStack call help us here?

[ncoghlan]

Note that the cleanup isn't actually conditional on whether or not the function had to create its own TemporaryDirectory instance - the function always calls requirements_dir.cleanup(), even if the requirements directory was passed in from outside.

So even though this is definitely a use case that ExitStack was designed to handle (via ExitStack.callback), given that pipenv supports Python 2.7 and doesn't otherwise depend on contextlib2, I think a custom context manager would be a better fit for the situation:

@contextlib.contextmanager
def call_cleanup(obj):
    try:
        yield obj
     finally:
         obj.cleanup()

Alternatively, if we can rely on the passed in requirements_dir object always being a context manager in its own right (the same way that pipenv._compat.TemporaryDirectory is), then there isn't even a need for a wrapper, we can just write:

with requirements_dir:
    ...

[thernstig]

edit I wrote a new issue instead since it seems to be a separate issue: #3054

This just happened for us and we are not sure how to work around this. Any workarounds available?

hostA(master)>pip --version
pip 18.1 from /project/python2.7.1/lib/python2.7/site-packages/pip (python 2.7)
hostA(master)>pipenv --version
pipenv, version 2018.10.13
hostA(master)>pipenv install --dev
Installing dependencies from Pipfile.lock (beebde)…
Traceback (most recent call last):▉▉▉▉▉ 0/15 — 00:00:00
  File "/project/python2.7.1/bin/pipenv", line 11, in <module>
    sys.exit(cli())
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/decorators.py", line 64, in new_func
    return ctx.invoke(f, obj, *args, **kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/cli/command.py", line 249, in install
    editable_packages=state.installstate.editables,
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/core.py", line 1976, in do_install
    skip_lock=skip_lock,
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/core.py", line 1283, in do_init
    pypi_mirror=pypi_mirror,
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/core.py", line 784, in do_install_dependencies
    trusted_hosts=trusted_hosts
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/core.py", line 1435, in pip_install
    c = delegator.run(pip_command, block=block, env=pip_config)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/delegator.py", line 317, in run
    c.run(block=block, binary=binary, cwd=cwd, env=env)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/delegator.py", line 198, in run
    s = PopenSpawn(self._popen_args, **pexpect_kwargs)
  File "/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/pexpect/popen_spawn.py", line 53, in __init__
    self.proc = subprocess.Popen(cmd, **kwargs)
  File "/project/python2.7.1/lib/python2.7/subprocess.py", line 672, in __init__
    errread, errwrite)
  File "/project/python2.7.1/lib/python2.7/subprocess.py", line 1202, in _execute_child
    raise child_exception
TypeError: must be encoded string without NULL bytes, not str
/project/python2.7.1/lib/python2.7/site-packages/pipenv/vendor/vistir/compat.py:109: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/pipenv-f6EEoh-requirements'>
  warnings.warn(warn_message, ResourceWarning)
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 0/15 — 00:00:00

[techalchemy]

Ok this is actually becoming a problem, time to find a solution!