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

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

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!
What do you think ?
Flags: needinfo?(wlachance)
Flags: needinfo?(ahalberstadt)
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.
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.
(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.
Maybe that discussion should have happened on a separate bug, we are diverting a lot from the original summary now. :S
(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.
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
Flags: needinfo?(wlachance)
(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).
Bug 1173335 - [mozprofile] Profile should act as a context manager; r=wlach
Attachment #8620469 - Flags: review?(wlachance)
Great idea, I have no objections to this.
Flags: needinfo?(ahalberstadt)
I forgot to say that I generated the MozBase documentation, and my additions looks great to me.
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. :)
Attachment #8620469 - Flags: review?(wlachance)
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
Attachment #8620469 - Flags: review?(wlachance)
address Will review comments
Attachment #8621133 - Flags: review?(wlachance)
Attachment #8621133 - Flags: review?(wlachance) → review+
Comment on attachment 8621133 [details]
MozReview Request: address Will review comments

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

Ship It!
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!
Attachment #8620469 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/c0ef8de6093e
Assignee: nobody → j.parkouss
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.