Thanks for the feedback, everyone. Looks like the principle of re-using
connections is sound but this implementation is way wrong :-)
I'm not working much on this right now and will come back to it later,
but here's a dump of my current brain state.
Putting the sessions cache in svn_client_context_t
--------------------------------------------------
Yes, that's better. I had thought of that initially, but then I decided
it would not give a long-lived client enough control of the lifetime of
the sessions. The client would be able to clear the cache from time to
time, of course, but it would not be able to use one set of sessions for
one sub-task and another set of sessions for another sub-task. But I
now suppose there are better ways to manage that issue -- perhaps using
one svn_client_context_t per sub-task, or providing and using
libsvn_client public APIs for controlling the cache.
Implementation at the libsvn_ra layer
-------------------------------------
Implementing the "get a cached session" logic in libsvn_client is
problematic. In that first patch, in libsvn_client I implemented
replacement functions for a couple of high-level "open an RA session"
private APIs. But, as C-Mike pointed out, "RA sessions opened by client
functions have differing sets of callback functions implemented
depending on how svn_client__open_ra_session_internal() was called". We
don't always want to retrieve the exact same svn_ra_session_t that was
being used before, we want a different one that connects to the same
repository.
So this intervention would work better right at the bottom of
libsvn_client, where we call svn_ra_open4() inside
svn_client__open_ra_session_internal().
This requires support from libsvn_ra -- the ability to "re-initialize" a
given session object with a new set of callbacks and other config data.
Or maybe the functionality should be described as "taking over" a given
RA session for a new connection to the same repository.
So my current thinking is we need a new RA function alongside
svn_ra_open4(), that modifies the given session object (or logically
"closes" the old session object and returns a new one) to represent a
new session without going through a re-connection round-trip.
Pool clean-up vs. an explicit "release" function
------------------------------------------------
Having pool clean-up be the primary release mechanism is fine and good.
It ensures the release will happen at some point even if a function
exits with an error. Already, each caller that wants to explicitly
close the session is allocating it in a dedicated subpool and then
destroying that subpool. If we now want libsvn_client to perform
different clean-up actions such as returning the session to the cache
instead of closing it, the general solution is for libsvn_client's "open
RA session" function to register its clean-up function on the
caller-provided pool, but when it opens a new RA session allocate that
in a "cache" pool that has a long lifetime. (Or in a subpool of the
cache pool, if we want to be able to close the cached connections
independently.)
We can always implement a "close" function as well, if we think that is
a useful option for some callers. That can be implemented on top of the
pool clean-up method of closing, at the libsvn_ra level and/or the
libsvn_client level. That would be an independent enhancement.
Clearing the cache; controlling the life-time of the cached sessions
--------------------------------------------------------------------
There should be a libsvn_client public API to
- close down all the cached sessions in the svn_client_ctx_t
and probably also to
- close down a cached session to a specific repository
- Julian
Received on 2011-09-15 14:06:06 CEST