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 ?
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
(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
Great idea, I have no objections to this.
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. :)
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
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+
You need to log in before you can comment on or make changes to this bug.