Re: Thread issues in svnserve

On Thu, May 9, 2013 at 10:57 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 09.05.2013 18:43, Ivan Zhakov wrote:
>> On Thu, May 9, 2013 at 8:41 PM, Philip Martin
>> <philip.martin_at_wandisco.com> wrote:
>>> Branko Čibej <brane_at_wandisco.com> writes:
>>>
>>>> On 09.05.2013 17:14, Ivan Zhakov wrote:
>>>>>> Perhaps we have to look at the httpd code?
>>>>>>
>>>>> httpd doesn't create worker thread dynamically, so they can allocate
>>>>> apr_thread_t in global pool. Also it has dedicated win32 mpm that uses
>>>>> CreateThread Windows API directly.
>>>> I'm beginning to think that we need a global thread pool object that's
>>>> independent of actual connections, that we create at process startup and
>>>> that contains its own root pool for creating new threads.
>>> I don't think that solves the problem. We have a loop calling
>>> apr_thread_create so the pool passed to apr_thread_create has to be
>>> cleared or destroyed. We can only do that when both:
>>>
>>> A: the call itself has finished
>>> B: the new thread has started running
>>>
>>> The thread itself can't clear the pool (the current code) because that
>>> doesn't guarantee A. We can't use a subpool (Ivan's patch) because that
>>> doesn't guarantee B.
>>>
>>> I think we have to add some inter-thread communication and have the main
>>> thread track the worker threads in some way.
>
> Like an interlocked queue of thread-created events?
>
Another way add some kind of svn__shared_pool_t with atomic reference
counter and destroying attached pool when counter reaches zero.

Something like this:
[[[
svn__shared_pool_t * svn__shared_pool_attach(apr_pool_t *pool)
{
    svn__shared_pool_t sp = apr_pcalloc(pool);
   sp->counter = 1;
   sp->pool = pool;
}

void svn__shared_pool_addref(svn__shared_pool_t *sp)
{
   apr_atomic_increment(sp->counter);
}

void svn__shared_pool_release(svn__shared_pool_t *sp)
{
   if (apr_atomic_decrement(sp->counter) == 0)
   {
      svn_pool_destroy(sp->pool);
   }
}

Then we do the following:
connection_pool = svn_pool_create();
sp = svn__shared_pool_attach(connection_pool);
svn__shared_pool_addref(sp); // for worker thread
data->sp = sp;
apr_threadattr_create(connection_pool);
apr_thread_create(connection_pool, worker, data);

svn_shared_pool_release(sp);

void worker()
{
   /// to the work
   svn_shared_pool_release(sp);
}
]]]

>> Another way is to use OS call directly instead of using APR.
>
> I'd rather avoid that if at all possible. In theory we could use
> threaded svnserve on other platforms, not just Windows.
>
Agreed.

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com