You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2007/11/07 15:43:27 UTC
svn commit: r592764 - in /httpd/httpd/trunk: include/util_ldap.h
modules/ldap/util_ldap.c
Author: covener
Date: Wed Nov 7 06:43:26 2007
New Revision: 592764
URL: http://svn.apache.org/viewvc?rev=592764&view=rev
Log:
Stop registering a cleanup on each LDAP connection created, this cleanup was
never called because it's registered against pconf in the child. LDAP
connections are created in the child and not shared between children, so no
action should be required at child exit
Additionally, clarify comments around uldap_connection_cleanup()
Modified:
httpd/httpd/trunk/include/util_ldap.h
httpd/httpd/trunk/modules/ldap/util_ldap.c
Modified: httpd/httpd/trunk/include/util_ldap.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_ldap.h?rev=592764&r1=592763&r2=592764&view=diff
==============================================================================
--- httpd/httpd/trunk/include/util_ldap.h (original)
+++ httpd/httpd/trunk/include/util_ldap.h Wed Nov 7 06:43:26 2007
@@ -190,8 +190,7 @@
* Cleanup a connection to an LDAP server
* @param ldc A structure containing the expanded details of the server
* that was connected.
- * @tip This function is registered with the pool cleanup to close down the
- * LDAP connections when the server is finished with them.
+ * @tip The connection is unlocked and returned to the list of free connections
* @fn apr_status_t util_ldap_connection_cleanup(util_ldap_connection_t *ldc)
*/
APR_DECLARE_OPTIONAL_FN(apr_status_t,uldap_connection_cleanup,(void *param));
Modified: httpd/httpd/trunk/modules/ldap/util_ldap.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ldap/util_ldap.c?rev=592764&r1=592763&r2=592764&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ldap/util_ldap.c (original)
+++ httpd/httpd/trunk/modules/ldap/util_ldap.c Wed Nov 7 06:43:26 2007
@@ -171,9 +171,8 @@
/*
- * Clean up an LDAP connection by unbinding and unlocking the connection.
- * This function is registered with the pool cleanup function - causing
- * the LDAP connections to be shut down cleanly on graceful restart.
+ * Clean up an LDAP connection by unbinding and unlocking the connection,
+ * causing it to be returned to the list of free connections
*/
static apr_status_t uldap_connection_cleanup(void *param)
{
@@ -562,11 +561,6 @@
/* save away a copy of the client cert list that is presently valid */
l->client_certs = apr_array_copy_hdr(l->pool, st->client_certs);
-
- /* add the cleanup to the pool */
- apr_pool_cleanup_register(l->pool, l,
- uldap_connection_cleanup,
- apr_pool_cleanup_null);
if (p) {
p->next = l;
Re: svn commit: r592764 - in /httpd/httpd/trunk: include/util_ldap.h modules/ldap/util_ldap.c
Posted by Eric Covener <co...@gmail.com>.
On Nov 7, 2007 10:55 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On Wed, November 7, 2007 4:43 pm, covener@apache.org wrote:
>
> > Stop registering a cleanup on each LDAP connection created, this cleanup
> > was
> > never called because it's registered against pconf in the child. LDAP
> > connections are created in the child and not shared between children, so
> > no
> > action should be required at child exit
>
> How does this affect graceful restarts, particularly on threaded platforms?
I don't think there are any concerns here because the connections
exist independently in each child.
--
Eric Covener
covener@gmail.com
Re: svn commit: r592764 - in /httpd/httpd/trunk:
include/util_ldap.h modules/ldap/util_ldap.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Wed, November 7, 2007 4:43 pm, covener@apache.org wrote:
> Stop registering a cleanup on each LDAP connection created, this cleanup
> was
> never called because it's registered against pconf in the child. LDAP
> connections are created in the child and not shared between children, so
> no
> action should be required at child exit
How does this affect graceful restarts, particularly on threaded platforms?
Regards,
Graham
--
Re: svn commit: r592764 - in /httpd/httpd/trunk:
include/util_ldap.h modules/ldap/util_ldap.c
Posted by Graham Leggett <mi...@sharp.fm>.
On Wed, November 7, 2007 4:43 pm, covener@apache.org wrote:
> Stop registering a cleanup on each LDAP connection created, this cleanup
> was
> never called because it's registered against pconf in the child. LDAP
> connections are created in the child and not shared between children, so
> no
> action should be required at child exit
How does this affect graceful restarts, particularly on threaded platforms?
Regards,
Graham
--
Re: svn commit: r592764 - in /httpd/httpd/trunk: include/util_ldap.h modules/ldap/util_ldap.c
Posted by Eric Covener <co...@gmail.com>.
On Nov 14, 2007 3:26 PM, Ruediger Pluem <rp...@apache.org> wrote:
> Hm. Currently this works fine, but in r591488, you create a subpool of st->pool and
> save it in l->pool and I understood that it was your intention that this subpool
> will be cleared sometime in the future and at this point of time uldap_connection_cleanup
> seems to be important again. As I see no harm in registering the cleanup for this pool
> and as the person who introduces clearing this subpool will have forgotten about this cleanup
> I would think that reverting would be the best here.
I was afraid that leaving cleanup registered (on a pool that isn't
cleaned up) would be the more misleading option. It's not just a
matter of picking when to run the cleanup -- the cleanup function
itself (and the prior allocations) do not accomodate fully removing a
backend connection.
The details of the allocation were largely unimportant when the
cleanup was intended to be run at child exit (even though it wasn't
running at all) because this all storage allocated only in the child
I'm glad to revert && comment until such a time that it can be worked
out more fully (and yank backport) -- thanks for the review.
--
Eric Covener
covener@gmail.com
Re: svn commit: r592764 - in /httpd/httpd/trunk: include/util_ldap.h
modules/ldap/util_ldap.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/07/2007 03:43 PM, covener@apache.org wrote:
> Author: covener
> Date: Wed Nov 7 06:43:26 2007
> New Revision: 592764
>
> URL: http://svn.apache.org/viewvc?rev=592764&view=rev
> Log:
> Stop registering a cleanup on each LDAP connection created, this cleanup was
> never called because it's registered against pconf in the child. LDAP
> connections are created in the child and not shared between children, so no
> action should be required at child exit
>
> Additionally, clarify comments around uldap_connection_cleanup()
>
>
> Modified:
> httpd/httpd/trunk/include/util_ldap.h
> httpd/httpd/trunk/modules/ldap/util_ldap.c
>
> Modified: httpd/httpd/trunk/include/util_ldap.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_ldap.h?rev=592764&r1=592763&r2=592764&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/util_ldap.h (original)
> +++ httpd/httpd/trunk/include/util_ldap.h Wed Nov 7 06:43:26 2007
> @@ -190,8 +190,7 @@
> * Cleanup a connection to an LDAP server
> * @param ldc A structure containing the expanded details of the server
> * that was connected.
> - * @tip This function is registered with the pool cleanup to close down the
> - * LDAP connections when the server is finished with them.
> + * @tip The connection is unlocked and returned to the list of free connections
> * @fn apr_status_t util_ldap_connection_cleanup(util_ldap_connection_t *ldc)
> */
> APR_DECLARE_OPTIONAL_FN(apr_status_t,uldap_connection_cleanup,(void *param));
>
> Modified: httpd/httpd/trunk/modules/ldap/util_ldap.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ldap/util_ldap.c?rev=592764&r1=592763&r2=592764&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ldap/util_ldap.c (original)
> +++ httpd/httpd/trunk/modules/ldap/util_ldap.c Wed Nov 7 06:43:26 2007
> @@ -171,9 +171,8 @@
>
>
> /*
> - * Clean up an LDAP connection by unbinding and unlocking the connection.
> - * This function is registered with the pool cleanup function - causing
> - * the LDAP connections to be shut down cleanly on graceful restart.
> + * Clean up an LDAP connection by unbinding and unlocking the connection,
> + * causing it to be returned to the list of free connections
> */
> static apr_status_t uldap_connection_cleanup(void *param)
> {
> @@ -562,11 +561,6 @@
>
> /* save away a copy of the client cert list that is presently valid */
> l->client_certs = apr_array_copy_hdr(l->pool, st->client_certs);
> -
> - /* add the cleanup to the pool */
> - apr_pool_cleanup_register(l->pool, l,
> - uldap_connection_cleanup,
> - apr_pool_cleanup_null);
Hm. Currently this works fine, but in r591488, you create a subpool of st->pool and
save it in l->pool and I understood that it was your intention that this subpool
will be cleared sometime in the future and at this point of time uldap_connection_cleanup
seems to be important again. As I see no harm in registering the cleanup for this pool
and as the person who introduces clearing this subpool will have forgotten about this cleanup
I would think that reverting would be the best here.
Regards
Rüdiger