[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf

From: Greg Hudson <ghudson_at_MIT.EDU>
Date: 2006-01-27 18:53:11 CET

On Fri, 2006-01-27 at 09:24 -0800, Justin Erenkrantz wrote:
> The key for the subpool is to have serf's socket be in a separate pool
> from the main session pool. Without that (i.e. everything in one
> pool), the underlying socket gets cleared before the serf cleanup
> routines are called and it leads to a segfault. Placing serf in its
> own subpool resolves this problem.

Two criticisms:

  * APR documents that cleanups are run in reverse order of
registration, but I don't see any documentation that subpool cleanups
will be run after pool cleanups. So, unless I missed something, you
seem to have resolved this problem by relying on undocumented behavior,
when you could just register your cleanup after the socket is created
and rely on documented behavior.

  * Will you eventually need to do anything to the socket besides close
it? If not, why not just rely on the socket's own cleanup and do
nothing? (Speaking of which, setting serf_sess->conn to NULL feels like
rearranging deck chairs on a sinking ship; the sole cleanup operation
for a data structure does not need to worry too much about the field
values of that structure's fields just prior to its being freed.)

> (And, I'm trying to avoid using svn-isms for now.)

Why? I can see using Apache httpd conventions for an httpd module like
mod_authz_svn (although I don't agree with it), but this is pretty
squarely Subversion code.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 27 18:55:19 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.