Repository.close() not called / cleanup mechanism

[thomas.moschny@…]

It seems to me that Repository.close() is never called, especially not by the main users of the vc layer, i.e. versioncontrol/web_ui/{changeset.py|log.py|browser.py}.

In my vc backend I'd have to do some cleanup (closing a database connection) when the repository handle is not needed anymore. The close() method looks like the right place for doing such cleanups (in contrast to __del__() finalizers, which are highly unreliable).

Note that such cleanups have to be done always when trac ends, regardless of the serving method used! (For example, during development, I use tracd, and regularly kill it using a ctrl-c).

Thinking of it, trac could even have a shutdown extension point to be implemented by all plugins that need to do something when trac terminates. (Maybe it would be worth filing an enhancement ticket for that…)

[Christian Boos]

Proposed patch:

Index: trunk/trac/versioncontrol/api.py
===================================================================
--- trunk/trac/versioncontrol/api.py	(revision 5081)
+++ trunk/trac/versioncontrol/api.py	(working copy)
@@ -112,7 +112,9 @@
             assert tid == threading._get_ident()
             try:
                 self._lock.acquire()
-                self._cache.pop(tid, None)
+                repos = self._cache.pop(tid, None)
+                if repos:
+                    repos.close()
             finally:
                 self._lock.release()

Comments / tests welcomed!

[thomas.moschny@…]

The patch seems to work, and should be applied.

However, this does not solve my initial problem, see #5010.

[Christian Boos]

Applied in r5145.

This will eventually be backported to milestone:0.10.5, if it proves to be unproblematic for trunk after a while.

[thomas.moschny@…]

For symmetry reasons one would like to have a open() method as well. But it is not only symmetry: With the current situation, you must be very careful not to pass around (i.e. make a second reference to) an instance of a Repository and call close() on it, not knowing whether the party you gave the reference to is still using it.

If we had open() and close(), references could be passed around and copied without problem, as long as each user of such a reference always calls open() and close() pairwise, which is probably easier to verify.

If you look for example in source:sandbox/mercurial-plugin-0.11/tracext/hg/backend.py, these are obvious places for adding a close():

tracext/hg/backend.py

@@ -65 +65 @@
         def render_property(self, name, mode, context, props):
             revs = props[name]
             def link(rev):
-                chgset = context.env.get_repository().get_changeset(rev)
+                repos = context.env.get_repository()
+                chgset = repos.get_changeset(rev)
+                repos.close()
                 return tag.a(rev, class_="changeset",
                              title=shorten_line(chgset.message),
                              href=context.href.changeset(rev))
@@ -117 +119 @@
                     rev = head
                     break
         try:
-            chgset = repos.get_changeset(rev)
-            return tag.a(label, class_="changeset",
-                         title=shorten_line(chgset.message),
-                         href=formatter.href.changeset(rev))
-        except NoSuchChangeset, e:
-            return tag.a(label, class_="missing changeset",
-                         title=to_unicode(e), rel="nofollow",
-                         href=formatter.href.changeset(rev))
+            try:
+                chgset = repos.get_changeset(rev)
+                return tag.a(label, class_="changeset",
+                             title=shorten_line(chgset.message),
+                             href=formatter.href.changeset(rev))
+            except NoSuchChangeset, e:
+                return tag.a(label, class_="missing changeset",
+                             title=to_unicode(e), rel="nofollow",
+                             href=formatter.href.changeset(rev))
+        finally:
+            repos.close()
 
 ### Helpers
 

But what about the repos references passed to MercurialNode or MercurialChangeset? Who is responsible for calling close() on them?

[Christian Boos]

Replying to thomas.moschny@gmx.de:

For symmetry reasons one would like to have a open() method as well. But it is not only symmetry: With the current situation, you must be very careful not to pass around (i.e. make a second reference to) an instance of a Repository and call close() on it, not knowing whether the party you gave the reference to is still using it.

You should never call close() by yourself, unless you've created the Repository instance. But for the one obtained by calling env.get_repository(), you don't have to and you shouldn't call close() by yourself because the lifetime of such objects are not under your control. Admittedly, this is another place where the API doc can be improved ;-)

(…) mercurial-plugin-0.11 (…) these are obvious places for adding a close():

No, because the assumption is that opening and closing a repository is not a cheap operation. We wouldn't do an open/close only for producing a single changeset TracLink, for example.

That's why we currently do it once per request, and only for the non-Chrome requests, see the RepositoryManager in versioncontrol/api.py. This is also the Component taking care of doing the close(), in … the shutdown(tid) method ;-)

[thomas.moschny@…]

Replying to cboos:

You should never call close() by yourself, unless you've created the Repository instance.

Well, but the RepositoryManager doesn't create the Repository instance either!

It merely calls get_repository() on one of the connectors, and later, on shutdown, calls close() on the repository. The implication is that the connector has to be careful to not give out the same repository reference twice, which means it must always create new repositories, and repositories cannot be used by more than one thread.

But, if one wants to cache repositories in the connector, this could e.g. be done by implementing a reference counter (in the repository handle), which is incremented by the connector (via -tada!- repos.open() ;) ) when it gives out repos, and decremented again in repos.close(). However, one could argue that this is an internal design detail of the plugin.