Skip to content

Fix Inkscape cleanup at exit on Windows. #19811

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

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 29, 2021

PR Summary

Closes #19809, at least locally (@jungerm2 can you confirm it works for you?)
I'd rather not figure out how to mess with CI to get Inkscape on Windows as well...

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jungerm2
Copy link

Confirmed! It works for me (at least locally).

Could you explain a bit how you came up with this specific solution? I understand that the process was keeping the tmpdir alive but still unsure of why the __del__ wasn't called properly (or didn't let the tmpdir be gc'ed). I had some trouble debugging this issue myself, so any tips/pointers are appreciated. Thanks!

@anntzer
Copy link
Contributor Author

anntzer commented Mar 29, 2021

I'm not actually 100% sure of what's happening (especially wrt. the precise timing where __del__ gets called), but I believe the problem is that even when the Popen object gets gc'ed, that doesn't actually kill the underlying process (this can be checked e.g. on linux with python -c 'from subprocess import Popen; Popen("sleep 1; echo foo", shell=True)': "foo" is printed after python exited (so certainly the Popen object has been gc'ed at that point).
Therefore, when the _SVGConverter gets gc'ed, both _tmpdir and _proc get gc'ed (in some non-specified order), but _tmpdir immediately wants to delete the directory, whereas the process managed by _proc will only terminate some time in the future, unless we explicitly both kill() and wait() it (from previous experience, even kill() is not enough -- there can be some small interval of time after kill() returns where the process hasn't actually exited yet).

@jungerm2
Copy link

This SO answer is helpful. Specifically, when using atexit, "what happens if a class is garbage collected before exit"? It seems that we should be able to perform cleanup solely with finalize and skip the atexit as when this is called isn't always well defined wrt the class's lifespan.

But then again there's definitely a use for atexit as indicated by the comment:

        # Explicitly register deletion from an atexit handler because if we
        # wait until the object is GC'd (which occurs later), then some module
        # globals (e.g. signal.SIGKILL) has already been set to None, and
        # kill() doesn't work anymore...

Using both seems unnecessary/redundant/prone to error as they are calling the same __del__ method at unknown times in the future.


Of note too is this section from the docs:

Note: It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj. 

So it seems that the function that finalize calls shouldn't be __del__ but instead a static/classmethod or a regular old function.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 30, 2021

To be clear I'm not claiming my solution is best, just that it works empirically... but if you have something better, please open a PR :-)

@jungerm2
Copy link

Yeah, absolutely, this seems to work and should be merged. I don't have a better solution, I was just curious as to if one exists/ how the atexit and finalize interplay.

To be fair, the "best" solution would be to use these classes as context managers with with, but that's hardly necessary...

@tacaswell tacaswell added this to the v3.4.1 milestone Mar 31, 2021
@tacaswell tacaswell merged commit 45e3d1f into matplotlib:master Mar 31, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 31, 2021
@anntzer anntzer deleted the svgcleanup branch March 31, 2021 05:26
QuLogic added a commit that referenced this pull request Mar 31, 2021
…811-on-v3.4.x

Backport PR #19811 on branch v3.4.x (Fix Inkscape cleanup at exit on Windows.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests that use "image_comparison" fail to cleanup on Windows
4 participants