912200 - [mozprofile] Refine mozprofile cleanup strategy

Status: RESOLVED WONTFIX

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

See https://bugzilla.mozilla.org/show_bug.cgi?id=911218

Currently, Profile cleans up on __del__ which is set to the idempotent
`cleanup` method:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L246

This in turn calls the cleanup methods of AddonManager amongst others
IFF self.restore is true.

To contrast, AddonManager.clean_addons cleans the added addons
regardless of self.restore; instead, __del__ conditionally calls
`clean_addons` IFF self.restore is set:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L277

This approach has a few disadvantages:
- when __del__ is called is unreliable and should not be a prereq for
using mozprofile
- the approaches above are inconsistent
- Profile exposes no way of explicitly cleaning up if self.restore is
false
- in the case of temporary profiles (that is, profiles created by the
`Profile` instance), the only cleanup really required is directory
deletion

Though there are a couple of advantages:
- the cleanup methods are idempotent; if you cleanup more than once,
nothing bad happens
- the cleanup methods are explicitly callable: if you want to
guarantee that cleanup happens at a certain time, you can.
- while not foolproof -- __del__ is not guarnateed to be called -- the
intention is that mozprofile should simply clean up after itself
unless it is told to persist the data.  While one point of view
states that a harness should always cleanup profiles, having Profile
clean up after itself prevents some classes of user error, including
not cleaning up after e.g. an exception is raised.

Ideally, all the advantages would be kept and the disadvantages
mitigated.

Proposal:

- mozprofile originally supported python 2.4.  Now that we require
  2.7, we can use contextmanagers as suggested in
  https://bugzilla.mozilla.org/show_bug.cgi?id=911218#c3 .  In this
  case, if Profile/AddonManager etc is instantiated as a
  contextmanager (that is, with `with`:
  http://docs.python.org/2/reference/compound_stmts.html#the-with-statement )
  then it cleans up after itself; otherwise, we require .clean to
  explicitly be called.

- move mozprofile classes to the methodology of AddonManager; that is,
  the cleanup method should always cleanup e.g. the added stuff and
  __exit__ calls cleanup (potentially conditionally depending on
  self.restore).

- the Profile class should cleanup temporary profiles with just an
  rmtree call (I think!)

Questions:

- The above supports implicit cleanup: when a profile goes out of
  scope, cleanup should happen.  However, we don't have to support
  this if we want to burden the harness with the responsibility.

- Is self.restore needed in the context manager case?

- ...If not, do we care about the case where we want an instance
  variable to be cleaned up?  that is, you can do

  def foo():
    with bar() as fleem:
      something()

  But I don't know how to do e.g.:

  class Foo(object):
    def __init__(self):
      self.bar = bar()
    def method1(self):
      self.bar.method2()

  ...with contextmanagers in any way that guarantees cleanup.

Comment 1

[Geoff Brown [:gbrown]]

Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.