Moviepy: __del__ is insufficient - need a close()

[Julian-O]

I am working on refactoring some code where the original author uses moviepy. So, I am not too familiar with the code usage, sorry.

I am having some problems that echo the problems described in #101, #110, #164, and #518. Examining the code, I believe they all have the same root cause.

Various classes (e.g. VideoFileClip) follow the same pattern:

• In the constructor they seize some resources (e.g. creating FFMPEG_VideoReader, which creates a process which locks the file.
• In the __del__ method, they release these resources (by calling del on sub-resources, and by terminating any subprocesses.).

I believe there are two misunderstandings here.

• Python's __del__ is not like C++'s destructors. There is no guarantee that they run immediately - or that they run at all. In practice, even though my code is deleting the VideoFileClip instance, the file remains open until the garbage collector gets around to it.
• On Windows, unlike Linux, you cannot delete a file that is in use by another process. So when Windows users complain that the file is still locked, the Linux developers are saying "It seems to work fine."

As a result, you are seeing Windows users complain that temporary files can't be deleted, and other users complaining that the number of "zombie" subprocesses is climbing as they continue processes.

The solution is:

• Don't rely on garbage-collection to manage the lifetime of non-memory resources. Add a close() method to each of the items that has resources, so clients can call them. By all means, continue to support __del__for when clients forget and the garbage collector notices.
• Please, while doing this, be even more pythonic: Support the context manager interface (__enter__ and __exit__), so people can write code in a way that close is always called, and the resources are always cleaned up, and it happens when they expect it.

---

The symptoms I am seeing, broadly:

Running Windows 10, Python 3.6.0. moviepy==0.2.3.2

• I create a file and put video data in it.
• I pass it to VideoFileClip(), and then use the instance to extract dimension data.
• I delete the VideoFileClip instance.
• I try to delete the file, but Windows complains it is still being used by another process.
• Then the __del__ methods are called by the garbage collector, during shut-down. (I know this, because I added some print statements to the moviepy code.

[gyglim]

I second that. I'd love to have a context manager that takes care of the cleanup.
My current solution is to explicitly calling __del__(), which is not very nice

[Zulko]

Thanks very much for the insight @Julian-O. Let's maybe wait for one or two other opinions, but if it was all good, would you be able to clean the code and submit a PR ?

[earney]

I agree.. I'd like to see if his suggestions will fix the issue. I don't see any harm in any of the things he is proposing.

[Julian-O]

@gyglim: Yes, I considered explicitly calling __del__() on VideoFileClip, but if you look at the code, it does nothing but call del on its subresources (which doesn't happen immediately), so the problem still remains.

[Julian-O]

I am underway with this change.

Here is my current understanding of how it will work.

Do you think we will be able to clearly explain to users which instances they need to call close() on, and when calling close() will also make the copies fail?

[Julian-O]

I still haven't touched the documentation. I don't want to pollute simple introductory examples with complexity they don't need, but I also want to demonstrate good, clean code that uses close() correctly.

[Julian-O]

Please tag a look at these changes, and tell me how you'd like to proceed. They have been swallowed up in a separate PR.

[Zulko]

@Julian-O @earney Sorry for the late reaction, but is this issue solved at least on Gtihub now ? Looking at this thread I understand that there is an old PR pending. As there were two issues related to ffmpeg closing recently, what's the general opinion on merging @Julian-O 's solution ? (I haven't had time to review it all ad think of side effects but looks good at first sight)

[Julian-O]

@Zulko: I've been using my fork with the patches (and other patches documented in other pull requests) included, and it is passing tests and working fine for me. So, yes, it has been solved somewhere on GitHub, but not in the main branch yet.

[mgaitan]

sorry guys, which is the PR mentioned? close was implemented in 02fc129f, and that means this could be closed, right?

[keikoro]

@Julian-O @mgaitan Any update on this?

[Julian-O]

@mgaitan, @keikoro:

The core functionality has been merged. However, some trivial formatting, spelling errors and the like remain unmerged in PR#608. It is sitting there, (forgotten?) but it isn't important enough to wave a flag about.

[keikoro]

Linking this as I didn't see it linked upthread #608

[keikoro]

@Julian-O Hmm, I see that one of the checks failed and that there was no follow-up on the code review (e.g. in form of a new commit), maybe that's why no-one continued looking into this