1173335 - [mozprofile] Profile should act as a context manager (using the with statement)

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Julien Pagès (:parkouss)]

Currently working on bug 999009, there is some corner cases where a cloned profile is not deleted. Anyway, relying on __del__ to clean the profile is not really safe (I'm sure others had issues!), but ensuring a call to the cleanup method is a pain.

So I suggest that we make the profile act as a context manager, so it can automatically call the cleanup method:

with profile:
   # do stuff

# in any case, profile.cleanup() has been called

Note that it won't break the current api, just add a new way to use the profile.

I can work on it if someone else is interested!

Comment 1

[Julien Pagès (:parkouss)]

What do you think ?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The problem with __del__ is that it is only called if all references to the object are gone. If we somewhere leak a reference we will miss to delete the profile, correct. I wonder which code path you were using which caused this situation. Beside this proposal we should clearly fix it.

Comment 3

[Julien Pagès (:parkouss)]

Sure. This is what I tried:

https://github.com/parkouss/mozregression/tree/mozprofile-leak

The interesting patch is in https://github.com/parkouss/mozregression/commit/f002b61b4de21d199742cd340087fbf6837724b1

note that this only affect:

https://github.com/parkouss/mozregression/blob/f002b61b4de21d199742cd340087fbf6837724b1/mozregression/launchers.py#L110
https://github.com/parkouss/mozregression/blob/f002b61b4de21d199742cd340087fbf6837724b1/mozregression/launchers.py#L192

where I only keep the reference to the profile as a local variable in each function. I am sure the cleanup is not called at least for the second call (in FennecLauncher). You can try it with:

git clone https://github.com/parkouss/mozregression
cd mozregression
git checkout mozprofile-leak
virtualenv -p /usr/bin/python2 venv
. venv/bin/activate
pip install mock
pip install -e .
python setup.py test

That will run the tests, you will have three failures, this is not a problem, but also two folders in /tmp that are not deleted...

Note that even If I fix the tests so they do not break anymore (just removing the assert calls), I still have the two folders that
are not deleted.

If you remove the last commit, the folders are correctly removed.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julien Pagès from comment #3)
> The interesting patch is in
> https://github.com/parkouss/mozregression/commit/
> f002b61b4de21d199742cd340087fbf6837724b1

Looking at this patch I see that you pass in a profile instance to self.profile_class.clone(). But this method expects a class name as first parameter instead. 

> https://github.com/parkouss/mozregression/blob/
> f002b61b4de21d199742cd340087fbf6837724b1/mozregression/launchers.py#L110
> https://github.com/parkouss/mozregression/blob/
> f002b61b4de21d199742cd340087fbf6837724b1/mozregression/launchers.py#L192

You can add some refcounting debug code to check where the leak actually is:
https://docs.python.org/2/library/sys.html#sys.getrefcount

Please let me know if that was the correct assumption.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe that discussion should have happened on a separate bug, we are diverting a lot from the original summary now. :S

Comment 6

[Julien Pagès (:parkouss)]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Looking at this patch I see that you pass in a profile instance to
> self.profile_class.clone(). But this method expects a class name as first
> parameter instead. 

Ah I should have mentionned that, but this is just the path to an existing profile.

Comment 7

[William Lachance (:wlach)]

This sounds good to me and should be trivial to add. ++ from me.

It would be great if you could update the docs with an example of how to do this at the same time. I noticed that we don't actually have a general example of how to use mozprofile in the docs at this time, this would be a great opportunity to add one.

http://mozbase.readthedocs.org/en/latest/mozprofile.html

Comment 8

[Julien Pagès (:parkouss)]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Maybe that discussion should have happened on a separate bug, we are
> diverting a lot from the original summary now. :S

Right! I did that with bug 1173380 (also reduced the test case).

Comment 9

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

Comment 10

[Andrew Halberstadt [:ahal]]

Great idea, I have no objections to this.

Comment 11

[Julien Pagès (:parkouss)]

I forgot to say that I generated the MozBase documentation, and my additions looks great to me.

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8620469 [details]
MozReview Request: Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

https://reviewboard.mozilla.org/r/10775/#review9513

::: testing/mozbase/mozprofile/mozprofile/profile.py:30
(Diff revision 1)
> +    The Profile should be cleaned automatically when it is garbage

I think saying something like "The files associated with the profile will be removed automatically after the object is garbage collected" would be more clear.

::: testing/mozbase/mozprofile/mozprofile/profile.py:39
(Diff revision 1)
> +    profile is cleaned by using it as a context manager: ::

I think this wording might be a bit more clear: 

cleanup is called under the hood to remove the profile files. You can ensure this method is called (even in the case of exception) by using the profile as a context manager:

Just a few documentation nits. Looks good otherwise. :)

Comment 13

[Julien Pagès (:parkouss)]

Comment on attachment 8620469 [details]
MozReview Request: Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

Comment 14

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: address Will review comments

address Will review comments

Comment 15

[William Lachance (:wlach)]

Comment on attachment 8621133 [details]
MozReview Request: address Will review comments

https://reviewboard.mozilla.org/r/10901/#review9523

Ship It!

Comment 16

[William Lachance (:wlach)]

Comment on attachment 8620469 [details]
MozReview Request: Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach

https://reviewboard.mozilla.org/r/10775/#review9533

Ship It!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ef8de6093e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0ef8de6093e