Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#4972 closed defect (fixed)

Repository.close() not called / cleanup mechanism

Reported by: thomas.moschny@… Owned by: Christian Boos
Priority: normal Milestone: 0.11
Component: version control Version: devel
Severity: normal Keywords:
Cc:
Release Notes:
API Changes:

Description

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…)

Attachments (0)

Change History (6)

comment:1 Changed 12 years ago by 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!

comment:2 Changed 12 years ago by thomas.moschny@…

The patch seems to work, and should be applied.

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

comment:3 Changed 12 years ago by Christian Boos

Resolution: fixed
Status: newclosed

Applied in r5145.

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

comment:4 Changed 12 years ago by 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

     
    6565        def render_property(self, name, mode, context, props):
    6666            revs = props[name]
    6767            def link(rev):
    68                 chgset = context.env.get_repository().get_changeset(rev)
     68                repos = context.env.get_repository()
     69                chgset = repos.get_changeset(rev)
     70                repos.close()
    6971                return tag.a(rev, class_="changeset",
    7072                             title=shorten_line(chgset.message),
    7173                             href=context.href.changeset(rev))
     
    117119                    rev = head
    118120                    break
    119121        try:
    120             chgset = repos.get_changeset(rev)
    121             return tag.a(label, class_="changeset",
    122                          title=shorten_line(chgset.message),
    123                          href=formatter.href.changeset(rev))
    124         except NoSuchChangeset, e:
    125             return tag.a(label, class_="missing changeset",
    126                          title=to_unicode(e), rel="nofollow",
    127                          href=formatter.href.changeset(rev))
     122            try:
     123                chgset = repos.get_changeset(rev)
     124                return tag.a(label, class_="changeset",
     125                             title=shorten_line(chgset.message),
     126                             href=formatter.href.changeset(rev))
     127            except NoSuchChangeset, e:
     128                return tag.a(label, class_="missing changeset",
     129                             title=to_unicode(e), rel="nofollow",
     130                             href=formatter.href.changeset(rev))
     131        finally:
     132            repos.close()
    128133
    129134### Helpers
    130135

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

comment:5 in reply to:  4 ; Changed 12 years ago by 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 ;-)

comment:6 in reply to:  5 Changed 12 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.