You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by mt...@apache.org on 2008/08/06 11:25:53 UTC
svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Author: mturk
Date: Wed Aug 6 02:25:52 2008
New Revision: 683191
URL: http://svn.apache.org/viewvc?rev=683191&view=rev
Log:
Do not serialize destructor calls.
Modified:
apr/apr-util/trunk/misc/apr_reslist.c
Modified: apr/apr-util/trunk/misc/apr_reslist.c
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/misc/apr_reslist.c?rev=683191&r1=683190&r2=683191&view=diff
==============================================================================
--- apr/apr-util/trunk/misc/apr_reslist.c (original)
+++ apr/apr-util/trunk/misc/apr_reslist.c Wed Aug 6 02:25:52 2008
@@ -452,10 +452,11 @@
void *resource)
{
apr_status_t ret;
+
+ ret = reslist->destructor(resource, reslist->params, reslist->pool);
#if APR_HAS_THREADS
apr_thread_mutex_lock(reslist->listlock);
#endif
- ret = reslist->destructor(resource, reslist->params, reslist->pool);
reslist->ntotal--;
#if APR_HAS_THREADS
apr_thread_cond_signal(reslist->avail);
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-08-06 at 12:27 +0200, Mladen Turk wrote:
> The major problem with invalidate is that blocking
> destructors (socket shutdown for example)
> are serialized, meaning that closing 100 sockets
> is done one at a time instead by owning threads in
> parallel.
Why don't you instead shut the socket down from your code just before
calling apr_reslist_invalidate() and set a flag inside the resource
itself that things have already been cleaned. Then the destructor
(although serialised) will run fast. Like this:
----------------
thread:
socket-shutdown
already-cleaned-up=true
apr_reslist_invalidate
destructor:
if already-cleaned-up
return
socket-shutdown
----------------
--
Bojan
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2008-08-06 at 12:46 +0200, Ruediger Pluem wrote:
> I am worried about the case that the destructor might allocate
> memory from the pool for temporary purposes (ok that would be a
> memory leak in the long run).
Not only that, but if you have a look at testreslist.c, you'll find
this:
----------------------------
static apr_status_t my_destructor(void *resource, void *params,
apr_pool_t *pool)
{
my_resource_t *res = resource;
my_parameters_t *my_params = params;
res->id = my_params->d_count++;
apr_sleep(my_params->sleep_upon_destruct);
return APR_SUCCESS;
}
----------------------------
The line "res->id = my_params->d_count++;" is updating a common
structure. Previously, this would be done under a lock in all
circumstances. With the change, this would no longer be the case. This
has potential to cause havoc in existing code. No?
--
Bojan
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Ruediger Pluem wrote:
>>
>> Hmm, might be. Although the pool create/destroy
>> is thread safe.
>
> I am worried about the case that the destructor might allocate
> memory from the pool for temporary purposes (ok that would be a
> memory leak in the long run).
>
Right. Anything except subpool create/destroy would be
memory leak, and that is thread safe.
Regards
--
^(TM)
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/06/2008 12:27 PM, Mladen Turk wrote:
> Ruediger Pluem wrote:
> >
> >
> > This looks dangerous to me. The destructor gets the reslist pool
> handed over
> > and pools are not thread save. See also:
> >
>
> Hmm, might be. Although the pool create/destroy
> is thread safe.
I am worried about the case that the destructor might allocate
memory from the pool for temporary purposes (ok that would be a
memory leak in the long run).
Regards
Rüdiger
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Mladen Turk <mt...@apache.org>.
Ruediger Pluem wrote:
>
>
> This looks dangerous to me. The destructor gets the reslist pool
handed over
> and pools are not thread save. See also:
>
Hmm, might be. Although the pool create/destroy
is thread safe.
The major problem with invalidate is that blocking
destructors (socket shutdown for example)
are serialized, meaning that closing 100 sockets
is done one at a time instead by owning threads in
parallel.
Regards
--
^(TM)
Re: svn commit: r683191 - /apr/apr-util/trunk/misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/06/2008 11:25 AM, mturk@apache.org wrote:
> Author: mturk
> Date: Wed Aug 6 02:25:52 2008
> New Revision: 683191
>
> URL: http://svn.apache.org/viewvc?rev=683191&view=rev
> Log:
> Do not serialize destructor calls.
>
> Modified:
> apr/apr-util/trunk/misc/apr_reslist.c
>
> Modified: apr/apr-util/trunk/misc/apr_reslist.c
> URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/misc/apr_reslist.c?rev=683191&r1=683190&r2=683191&view=diff
> ==============================================================================
> --- apr/apr-util/trunk/misc/apr_reslist.c (original)
> +++ apr/apr-util/trunk/misc/apr_reslist.c Wed Aug 6 02:25:52 2008
> @@ -452,10 +452,11 @@
> void *resource)
> {
> apr_status_t ret;
> +
> + ret = reslist->destructor(resource, reslist->params, reslist->pool);
> #if APR_HAS_THREADS
> apr_thread_mutex_lock(reslist->listlock);
> #endif
> - ret = reslist->destructor(resource, reslist->params, reslist->pool);
> reslist->ntotal--;
> #if APR_HAS_THREADS
> apr_thread_cond_signal(reslist->avail);
This looks dangerous to me. The destructor gets the reslist pool handed over
and pools are not thread save. See also:
/**
* Destroy a single idle resource.
* Assumes: that the reslist is locked.
*/
static apr_status_t destroy_resource(apr_reslist_t *reslist, apr_res_t *res)
{
return reslist->destructor(res->opaque, reslist->params, reslist->pool);
}
Regards
Rüdiger