shutdown hook for plugins

[thomas.moschny@…]

In a plugin, I'd like to create persistent connections somewhere (to a database, or a pipe to talk to another program, whatever) when trac starts (or my plugin is used for the first time), and close that connection again when trac ends, regardless of the reason why trac shuts down.

There could be an extension point resp. interface for the plugin to implement, that contains a shutdown() method.

[Christian Boos]

The DatabaseManager and the RepositoryManager components are already implementing a shutdown() method, which takes an optional tid argument, so I propose to reuse those conventions:

class IEnvironmentChangeReactor(Interface):
    """Extension point interface for components that need to maintain
    external resources and should be notified at appropriate time
    when they can release their resources."""

    def shutdown(tid=None):
        """Called when the Trac environment shuts down.

        When `tid` is not `None` this signals the termination of the
        corresponding thread instead of a full environment shutdown."""

[thomas.moschny@…]

• Is it possible that shutdown(tid) with tid != None is called separately for each thread? How does the plugin know when the last threads shuts down in this case?

• More generally: Does tid have any meaning to the plugin at all? Or is the plugin required to remember by itself all thread ids using it?

[Christian Boos]

shutdown(tid) is called only in the tid thread of course, just before it exists. It's up to the plugin to register some resources per thread (e.g. on first call) or not, and then evntually do the clean-up per-thread. If you prefer to do a global (per-proces) resource management, simply consider only the case when tid is None and ignore the other calls.

Again, this is just how the DatabaseManager and the RepositoryManager currently work, except that the calls to shutdown are hard-coded instead of being dispatched to the registered implementations of an extension point (see shutdown in source:trunk/trac/env.py​).

[thomas.moschny@…]

Sorry, but I don't get it (yet).

Say, we have exactly threads running, with tid=1, tid=2, and tid=3. Now, when the third thread ends, shutdown(3) would be called , similarly shutdown(2) for the second thread, and shutdown(None) for the first thread iff it ends last?

Nevertheless, factoring out the shutdown() method into an extension point sounds like a good idea, and matches exactly with the intention of this ticket!

[Christian Boos]

Currently, the web request dispatcher calls env.shutdown(threading._get_ident()) after a request is processed (see source:trunk/trac/web/main@5125#L435).

So in you example above, shutdown(3) and shutdown(2) will be called, but, as the last thread to exit is also the main thread, shutdown(1) wont be called, as this was not a thread created to serve a request. For now, shutdown(None) won't be called either, as we don't call env.shutdown on server termination. According to this proposal, this should be fixed, of course.

[anonymous]

To summarize, a component hooking into that extension point should, according to the proposal, free the ressources that are allocated specifically for the thread in which shutdown() is called (only if there are such ressources, of course); and should additionally free all global ressources if tid==None, right?

If this is correct, then it seems to me that tid serves only as a marker, and could be replaced by a boolean named last_thread, main_thread or similar. The existing code has assert tid==threading._get_ident(), so fetches its own thread id anyway, so doesn't really need the value of the tid argument, besides the fact whether it is None or not.

Otherwise, the proposal sounds fine. I should emphasize however, that I think the shutdown() methods should be called regardless of the reason the server terminates, preferably even in case of an keyboard interrupt or other exceptions.

Will the proposal work with trac running on mod_python and similar methods?

[thomas.moschny@…]

Oops, forgot to add my name to the last comment.

[Christian Boos]

Replying to anonymous:

To summarize, …

Yes, that's it.

If this is correct, then it seems to me that tid serves only as a marker, and could be replaced by a boolean named last_thread, main_thread or similar. The existing code has assert tid==threading._get_ident(), so fetches its own thread id anyway, so doesn't really need the value of the tid argument, besides the fact whether it is None or not.

Well, it's the assert tid==threading._get_ident() that can be seen as optional, as tid will be used as a key in a global cache. So passing tid can be seen as a convenience for not having to call threading._get_ident(). At least until we require Python 2.4, since at that point it will be possible to use Thread-local storage and the tid will become less useful.

I should emphasize however, that I think the shutdown() methods should be called regardless of the reason the server terminates, preferably even in case of an keyboard interrupt or other exceptions.

Yes.

Will the proposal work with trac running on mod_python and similar methods?

I hope so… those are things to be checked and done if possible. Even if there's no specific support for shutdown code, there would be the generic atexit handlers.

[anonymous]

Replying to cboos:

Well, it's the assert tid==threading._get_ident() that can be seen as optional, as tid will be used as a key in a global cache. So passing tid can be seen as a convenience for not having to call threading._get_ident().

Besides the fact that the last thread doesn't get passed a tid. Note that the plugins presumably might not know whether a thread is the "main" thread or one that was created in order to serve a request.

[Christian Boos]

For symmetry with shutdown(None), we could add a startup() to that interface as well (see #4190). That one won't be called for each thread. For per-request cache, it's enough and more effective to cache resources on first use.

The component's __init__ method is called

class IEnvironmentLifeCycleReactor(Interface):
    """Extension point interface for components that need to maintain
    external resources and should be notified at appropriate time
    when they can release their resources."""

    def startup():
        """Called immediately after the environment has been initialized."""

    def shutdown(tid=None):
        """Called when the Trac environment shuts down.

        When `tid` is `None`, this signals a full environment shutdown.
        Otherwise, `tid` is the identifier of a request serving thread, 
        which is about to terminate. This can be used to free resources
        cached for the life-time of that thread."""

[Christian Boos]

Forgot to finish comment:10:

The component's __init__ method is called …

… after the first use of the Component, which can happen at any time after the Environment creation.

So the point here is simply to go through the components implementing the IEnvironmentLifeCycleReactor interface immediately after the environment initialization. For those components, the startup() method will be called immediately after the __init__ method, so we could eventually get without an explicit startup() method here, but having one looks cleaner to me.

[thomas.moschny@…]

Replying to cboos:

For symmetry with shutdown(None), we could add a startup() to that interface as well (see #4190). That one won't be called for each thread. For per-request cache, it's enough and more effective to cache resources on first use.

So we would have these use cases (from the perspective of the plugin developer):

• startup() paired with shutdown(None): useful for global ressources.
• first usage paired with shutdown(None): for global ressources allocated on first usage.
• first usage, remember tid paired with shutdown(tid) or shutdown(None): for per-thread ressources.

To me, this looks like there are two interfaces interwoven: global startup() and shutdown(), and per (request serving) thread startup() and shutdown(), but anyway.

While it looks more symmetrical this way, I am not sure whether the first case is really useful. One point is that problems encountered on startup could be detected and reported to the user earlier than in the second case. Unfortunately, even #4190 fails to give a real use case. (The use case given there is itself a hack. It injects something into a Chrome instance, and as there's no hook for doing that in a sane way, there's the claim for startup().)

[Christian Boos]

Replying to thomas.moschny@gmx.de:

• first usage, remember tid paired with shutdown(tid) or shutdown(None): for per-thread ressources.

shutdown(None) is not paired with first usage remember tid here. It would be paired with the startup() (the first case).

shutdown(None) should be called on process exit only (that would be the main thread, and I don't think there's a need to care for the tid of the main thread).

[thomas.moschny@…]

Replying to cboos:

shutdown(None) should be called on process exit only (that would be the main thread, and I don't think there's a need to care for the tid of the main thread).

But how do my plugin's methods know whether they are called by the main thread or by a per-request thread?

I might still be misunderstanding something, but earlier, we agreed upon the fact that shutdown(tid) is not called for the main thread. But the main thread is also serving requests, isn't it? So it can of course call my plugin's methods. Again, how does the plugin know what thread is using it?

What I meant by "first usage, remember tid paired with shutdown(tid) or shutdown(None)" was, that on process exit, I have to free all ressources, regardless under which tid they were allocated.

[Christian Boos]

Well, no wonder everything is not entirely clear, as we're discussing how it should be, with only some aspects being currently coded (shutdown(tid!=None)), others being worked out!

I had mainly the tracd model in mind, where the main thread is not serving a request but simply calling HTTPServer.serve_forever(). A KeyboardInterrupt exception would certainly have to be handled at that level. Even in that model, the thread that would actually initialize an environment for the first time would not be that main thread, but a request serving thread.

With other front-ends, like the mod_python one, we're in the same situation: the thread that does the startup is a normal thread created for serving a request, and the one that will do the clean-up will be a different one (we have to possibility to register a cleanup callback, using ​register_cleanup()).

Here's some pseudo-Python code for describing the environment life-cycles:

env_cache = {}

def env_cleanup():
    for _, env in env_cache:
        env.shutdown()

def _open_environment(env_path):
    env = env_cache.get(env_path)
    if not env:
        env = env_cache[env_path] = open_environment(env_path)
        env.startup()
    return env

Here's the environment life-cycle for tracd:

# in main_thread: 

    try:
        main()
    finally:
        env_cleanup()

# in a thread serving a request:

   try:
       env = _open_environment()
       ... # process req
   finally:
       env.shutdown(tid)

and here's the environment life-cycle for mod_python:

cleanup_registered = False

# in a thread serving a request:

    if not cleanup_registered:
        apache.main_server.register_cleanup(req, env_cleanup)
        cleanup_registered = True
 
    try:
        env = _open_environment()
        ... # process req
    finally:
        env.shutdown(tid)

# rely on mod_python to call env_cleanup before the process exits

[thomas.moschny@…]

Hmm, I'd like to see some progress on this particular bug, especially as there are recurring rumors about a 0.11 release.

What is the next step to be done here? Would a patch be helpful?

[Christian Boos]

Replying to thomas.moschny@gmx.de:

Hmm, I'd like to see some progress on this particular bug, especially as there are recurring rumors about a 0.11 release.

That's no rumor, there will be a 0.11 release :p)

What is the next step to be done here? Would a patch be helpful?

Definitely. Also checking whether the proposed approach would work with cgi and fcgi, as I don't use those front-ends myself.

[ilias@…]

possibly related: #4190 (on core/component layer, environment independent)

[Christian Boos]

Keep this on the radar.

[Christian Boos]

Milestone 1.0 deleted