You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2013/05/05 16:36:06 UTC
Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)
On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <be...@reser.org> wrote:
>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
>> fetch the proposed tarballs from here:
>> https://dist.apache.org/repos/dist/dev/subversion
>>
> I've got crash in svnserve when running 32-bit test suite over svn://
> protocol on Windows 7 (x64).
[..]
>
> I'm running test suite again to check if this issue has stable reproduction.
>
The crash happened again.
It seems problem in apr_thread_* implementation and svnserve pool
management. Problem that we allocate apr_thread_t and apr_threadattr_t
in connection_pool which is destroyed when created thread completes.
This lead access to freed memory in apr_thread_create() when
connection thread completes very fast. See apr_thread_create()
implementation on Windows (apr\threadproc\win32\thread.c:82)
[[[
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
apr_threadattr_t *attr,
apr_thread_start_t func,
void *data, apr_pool_t *pool)
{
apr_status_t stat;
unsigned temp;
HANDLE handle;
(*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
if ((*new) == NULL) {
return APR_ENOMEM;
}
(*new)->data = data;
(*new)->func = func;
(*new)->td = NULL;
stat = apr_pool_create(&(*new)->pool, pool);
if (stat != APR_SUCCESS) {
return stat;
}
/* Use 0 for default Thread Stack Size, because that will
* default the stack to the same size as the calling thread.
*/
#ifndef _WIN32_WCE
if ((handle = (HANDLE)_beginthreadex(NULL,
(DWORD) (attr ? attr->stacksize : 0),
(unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
(*new), 0, &temp)) == 0) {
return APR_FROM_OS_ERROR(_doserrno);
}
#else
if ((handle = CreateThread(NULL,
attr && attr->stacksize > 0 ? attr->stacksize : 0,
(unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
(*new), 0, &temp)) == 0) {
return apr_get_os_error();
}
#endif
if (attr && attr->detach) {
^^^^^^^^^^^^^^ crash here because attr is allocated in
connection_pool which can be destroyed at this time
CloseHandle(handle);
}
else
(*new)->td = handle;
return APR_SUCCESS;
}
]]]
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 9, 2013 at 8:38 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Thu, May 9, 2013 at 8:10 PM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> But currently APR doesn't access apr_threadattr_t from worker thread
>>> and doesn't need access to apr_thread_t from main thread. So using
>>> iterpool for apr_threadattr_t and connection_pool for apr_thread_t
>>> fixes problem, but only with current APR implementation.
>>
>> I don't think it works. There are two problems. The first problem:
>>
> It works because apr_thread_create() currently doesn't access memory
> allocated in connection_pool after thread created.
>
Oops, this is true only for win32 implementation. unix implementation
allocated pthread_t structure from this pool.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 9, 2013 at 8:10 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> But currently APR doesn't access apr_threadattr_t from worker thread
>> and doesn't need access to apr_thread_t from main thread. So using
>> iterpool for apr_threadattr_t and connection_pool for apr_thread_t
>> fixes problem, but only with current APR implementation.
>
> I don't think it works. There are two problems. The first problem:
>
It works because apr_thread_create() currently doesn't access memory
allocated in connection_pool after thread created.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> I think we may be able to create non-detached threads and then
> explicitly detach:
Oops! I forgot to pass iterpool to apr_thread_create. Doesn't seem to
work when I fix it.
--
Philip
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> But currently APR doesn't access apr_threadattr_t from worker thread
> and doesn't need access to apr_thread_t from main thread. So using
> iterpool for apr_threadattr_t and connection_pool for apr_thread_t
> fixes problem, but only with current APR implementation.
I don't think it works. There are two problems. The first problem:
connection_pool = svn_pool_create(...)
apr_thread_create(... connection_pool)
Here the worker thread destroys connection_pool when the thread
completes. This can happen before apr_thread_create returns in which
case memory allocated inside apr_thread_create becomes invalid. This is
the problem you observed.
The second problem:
connection_pool = svn_pool_create(...)
subpool = svn_pool_create(...)
apr_thread_create(... subpool)
svn_pool_destroy(subpool)
Here the subpool can get destroyed before the thread starts. That means
that memory allocated inside apr_thread_create becomes invalid before
the thread starts. In that case the access to the memory in
dummy_worker is invalid.
I think we may be able to create non-detached threads and then
explicitly detach:
Index: subversion/svnserve/svnserve.c
===================================================================
--- subversion/svnserve/svnserve.c (revision 1480630)
+++ subversion/svnserve/svnserve.c (working copy)
@@ -455,6 +455,7 @@ int main(int argc, const char *argv[])
#if APR_HAS_THREADS
apr_threadattr_t *tattr;
apr_thread_t *tid;
+ apr_pool_t *iterpool;
struct serve_thread_t *thread_data;
#endif
@@ -973,6 +974,7 @@ int main(int argc, const char *argv[])
{
#if APR_HAS_THREADS
settings.single_threaded = FALSE;
+ iterpool = svn_pool_create(pool);
#else
/* No requests will be processed at all
* (see "switch (handling_mode)" code further down).
@@ -1101,14 +1103,6 @@ int main(int argc, const char *argv[])
svn_error_clear(err);
exit(1);
}
- status = apr_threadattr_detach_set(tattr, 1);
- if (status)
- {
- err = svn_error_wrap_apr(status, _("Can't set detached state"));
- svn_handle_error2(err, stderr, FALSE, "svnserve: ");
- svn_error_clear(err);
- exit(1);
- }
thread_data = apr_palloc(connection_pool, sizeof(*thread_data));
thread_data->conn = conn;
thread_data->params = ¶ms;
@@ -1122,6 +1116,15 @@ int main(int argc, const char *argv[])
svn_error_clear(err);
exit(1);
}
+ status = apr_thread_detach(tid);
+ if (status)
+ {
+ err = svn_error_wrap_apr(status, _("Can't detach thread"));
+ svn_handle_error2(err, stderr, FALSE, "svnserve: ");
+ svn_error_clear(err);
+ exit(1);
+ }
+ svn_pool_clear(iterpool);
#endif
break;
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Fri, May 10, 2013 at 1:16 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> 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:
>
> An implementation of your idea:
>
> Index: subversion/svnserve/svnserve.c
> ===================================================================
> --- subversion/svnserve/svnserve.c (revision 1480899)
> +++ subversion/svnserve/svnserve.c (working copy)
> @@ -53,6 +53,7 @@
>
> #include "private/svn_dep_compat.h"
> #include "private/svn_cmdline_private.h"
> +#include "private/svn_atomic.h"
>
> #include "winservice.h"
>
> @@ -378,11 +379,44 @@ static apr_status_t redirect_stdout(void *arg)
> return apr_file_dup2(out_file, err_file, pool);
> }
>
> +#if APR_HAS_THREADS
> +/* The pool passed to apr_thread_create can only be released when both
> +
> + A: the call to apr_thread_create has returned to the calling thread
> + B: the new thread has started running and reached apr_thread_start_t
> +
> + So we set the atomic counter to 2 then both the calling thread and
> + the new thread decrease it and when it reaches 0 the pool can be
> + released. */
> +struct shared_pool_t {
> + svn_atomic_t count;
> + apr_pool_t *pool;
> +};
> +
> +static struct shared_pool_t *
> +attach_shared_pool(apr_pool_t *pool)
> +{
> + struct shared_pool_t *shared = apr_palloc(pool, sizeof(struct shared_pool_t));
> +
> + shared->pool = pool;
> + svn_atomic_set(&shared->count, 2);
> +
> + return shared;
> +}
> +
> +static void
> +release_shared_pool(struct shared_pool_t *shared)
> +{
> + if (svn_atomic_dec(&shared->count) == 0)
> + svn_pool_destroy(shared->pool);
> +}
> +#endif
> +
> /* "Arguments" passed from the main thread to the connection thread */
> struct serve_thread_t {
> svn_ra_svn_conn_t *conn;
> serve_params_t *params;
> - apr_pool_t *pool;
> + struct shared_pool_t *shared_pool;
> };
>
> #if APR_HAS_THREADS
> @@ -390,8 +424,8 @@ static void * APR_THREAD_FUNC serve_thread(apr_thr
> {
> struct serve_thread_t *d = data;
>
> - svn_error_clear(serve(d->conn, d->params, d->pool));
> - svn_pool_destroy(d->pool);
> + svn_error_clear(serve(d->conn, d->params, d->shared_pool->pool));
> + release_shared_pool(d->shared_pool);
>
> return NULL;
> }
> @@ -455,6 +489,7 @@ int main(int argc, const char *argv[])
> #if APR_HAS_THREADS
> apr_threadattr_t *tattr;
> apr_thread_t *tid;
> + struct shared_pool_t *shared_pool;
>
> struct serve_thread_t *thread_data;
> #endif
> @@ -1093,6 +1128,7 @@ int main(int argc, const char *argv[])
> particularly sophisticated strategy for a threaded server, it's
> little different from forking one process per connection. */
> #if APR_HAS_THREADS
> + shared_pool = attach_shared_pool(connection_pool);
> status = apr_threadattr_create(&tattr, connection_pool);
> if (status)
> {
> @@ -1112,9 +1148,9 @@ int main(int argc, const char *argv[])
> thread_data = apr_palloc(connection_pool, sizeof(*thread_data));
> thread_data->conn = conn;
> thread_data->params = ¶ms;
> - thread_data->pool = connection_pool;
> + thread_data->shared_pool = shared_pool;
> status = apr_thread_create(&tid, tattr, serve_thread, thread_data,
> - connection_pool);
> + shared_pool->pool);
> if (status)
> {
> err = svn_error_wrap_apr(status, _("Can't create thread"));
> @@ -1122,6 +1158,7 @@ int main(int argc, const char *argv[])
> svn_error_clear(err);
> exit(1);
> }
> + release_shared_pool(shared_pool);
> #endif
> break;
>
>
Looks good!
My original idea was to make svn__shared_pool library private API for
better encapsulation, but we can do this anytime later. I'll vote for
backport on Monday.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> 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:
An implementation of your idea:
Index: subversion/svnserve/svnserve.c
===================================================================
--- subversion/svnserve/svnserve.c (revision 1480899)
+++ subversion/svnserve/svnserve.c (working copy)
@@ -53,6 +53,7 @@
#include "private/svn_dep_compat.h"
#include "private/svn_cmdline_private.h"
+#include "private/svn_atomic.h"
#include "winservice.h"
@@ -378,11 +379,44 @@ static apr_status_t redirect_stdout(void *arg)
return apr_file_dup2(out_file, err_file, pool);
}
+#if APR_HAS_THREADS
+/* The pool passed to apr_thread_create can only be released when both
+
+ A: the call to apr_thread_create has returned to the calling thread
+ B: the new thread has started running and reached apr_thread_start_t
+
+ So we set the atomic counter to 2 then both the calling thread and
+ the new thread decrease it and when it reaches 0 the pool can be
+ released. */
+struct shared_pool_t {
+ svn_atomic_t count;
+ apr_pool_t *pool;
+};
+
+static struct shared_pool_t *
+attach_shared_pool(apr_pool_t *pool)
+{
+ struct shared_pool_t *shared = apr_palloc(pool, sizeof(struct shared_pool_t));
+
+ shared->pool = pool;
+ svn_atomic_set(&shared->count, 2);
+
+ return shared;
+}
+
+static void
+release_shared_pool(struct shared_pool_t *shared)
+{
+ if (svn_atomic_dec(&shared->count) == 0)
+ svn_pool_destroy(shared->pool);
+}
+#endif
+
/* "Arguments" passed from the main thread to the connection thread */
struct serve_thread_t {
svn_ra_svn_conn_t *conn;
serve_params_t *params;
- apr_pool_t *pool;
+ struct shared_pool_t *shared_pool;
};
#if APR_HAS_THREADS
@@ -390,8 +424,8 @@ static void * APR_THREAD_FUNC serve_thread(apr_thr
{
struct serve_thread_t *d = data;
- svn_error_clear(serve(d->conn, d->params, d->pool));
- svn_pool_destroy(d->pool);
+ svn_error_clear(serve(d->conn, d->params, d->shared_pool->pool));
+ release_shared_pool(d->shared_pool);
return NULL;
}
@@ -455,6 +489,7 @@ int main(int argc, const char *argv[])
#if APR_HAS_THREADS
apr_threadattr_t *tattr;
apr_thread_t *tid;
+ struct shared_pool_t *shared_pool;
struct serve_thread_t *thread_data;
#endif
@@ -1093,6 +1128,7 @@ int main(int argc, const char *argv[])
particularly sophisticated strategy for a threaded server, it's
little different from forking one process per connection. */
#if APR_HAS_THREADS
+ shared_pool = attach_shared_pool(connection_pool);
status = apr_threadattr_create(&tattr, connection_pool);
if (status)
{
@@ -1112,9 +1148,9 @@ int main(int argc, const char *argv[])
thread_data = apr_palloc(connection_pool, sizeof(*thread_data));
thread_data->conn = conn;
thread_data->params = ¶ms;
- thread_data->pool = connection_pool;
+ thread_data->shared_pool = shared_pool;
status = apr_thread_create(&tid, tattr, serve_thread, thread_data,
- connection_pool);
+ shared_pool->pool);
if (status)
{
err = svn_error_wrap_apr(status, _("Can't create thread"));
@@ -1122,6 +1158,7 @@ int main(int argc, const char *argv[])
svn_error_clear(err);
exit(1);
}
+ release_shared_pool(shared_pool);
#endif
break;
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> 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);
> }
Yes, that looks promising.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 9, 2013 at 10:57 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 09.05.2013 18:43, Ivan Zhakov wrote:
>> On Thu, May 9, 2013 at 8:41 PM, Philip Martin
>> <ph...@wandisco.com> wrote:
>>> Branko Čibej <br...@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
Re: Thread issues in svnserve
Posted by Branko Čibej <br...@wandisco.com>.
On 09.05.2013 18:43, Ivan Zhakov wrote:
> On Thu, May 9, 2013 at 8:41 PM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Branko Čibej <br...@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 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.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 9, 2013 at 8:41 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Branko Čibej <br...@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.
>
Another way is to use OS call directly instead of using APR.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@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.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Branko Čibej <br...@wandisco.com>.
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.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Thread issues in svnserve
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 9, 2013 at 6:32 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> It might be better to pass iterpool instead of connection_pool into
>> apr_thread_create. The problem with tattr also applies to memory
>> allocated by apr_thread_create such as *new. At present *new is not
>> accessed after the thread has started but the implementation could
>> change. So new patch:
>
> No, that won't work. It's not possible to clear/destroy the pool passed
> to apr_thread_create until the thread has started (so that dummy_worker
> can access the memory) and there is no guarantee that will have happened
> when apr_thread_create returns.
>
> That makes pool handling a bit tricky. We can't use connection_pool
> because that goes wrong when the thread completes too quickly, and we
> can't use a subpool because that goes wrong when the thread starts too
> slowly.
>
Yes, that's problem.
But currently APR doesn't access apr_threadattr_t from worker thread
and doesn't need access to apr_thread_t from main thread. So using
iterpool for apr_threadattr_t and connection_pool for apr_thread_t
fixes problem, but only with current APR implementation. That's why I
call my fix workaround and didn't commit it to trunk.
> 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.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> It might be better to pass iterpool instead of connection_pool into
> apr_thread_create. The problem with tattr also applies to memory
> allocated by apr_thread_create such as *new. At present *new is not
> accessed after the thread has started but the implementation could
> change. So new patch:
No, that won't work. It's not possible to clear/destroy the pool passed
to apr_thread_create until the thread has started (so that dummy_worker
can access the memory) and there is no guarantee that will have happened
when apr_thread_create returns.
That makes pool handling a bit tricky. We can't use connection_pool
because that goes wrong when the thread completes too quickly, and we
can't use a subpool because that goes wrong when the thread starts too
slowly.
Perhaps we have to look at the httpd code?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> We could do this:
>
> Index: subversion/svnserve/svnserve.c
> ===================================================================
> --- subversion/svnserve/svnserve.c (revision 1480565)
> +++ subversion/svnserve/svnserve.c (working copy)
> @@ -444,6 +444,7 @@ int main(int argc, const char *argv[])
> apr_sockaddr_t *sa;
> apr_pool_t *pool;
> apr_pool_t *connection_pool;
> + apr_pool_t *iterpool;
> svn_error_t *err;
> apr_getopt_t *os;
> int opt;
> @@ -973,6 +974,7 @@ int main(int argc, const char *argv[])
> {
> #if APR_HAS_THREADS
> settings.single_threaded = FALSE;
> + iterpool = svn_pool_create(pool);
> #else
> /* No requests will be processed at all
> * (see "switch (handling_mode)" code further down).
> @@ -1093,7 +1095,7 @@ int main(int argc, const char *argv[])
> particularly sophisticated strategy for a threaded server, it's
> little different from forking one process per connection. */
> #if APR_HAS_THREADS
> - status = apr_threadattr_create(&tattr, connection_pool);
> + status = apr_threadattr_create(&tattr, iterpool);
> if (status)
> {
> err = svn_error_wrap_apr(status, _("Can't create threadattr"));
> @@ -1122,6 +1124,7 @@ int main(int argc, const char *argv[])
> svn_error_clear(err);
> exit(1);
> }
> + svn_pool_clear(iterpool);
> #endif
> break;
It might be better to pass iterpool instead of connection_pool into
apr_thread_create. The problem with tattr also applies to memory
allocated by apr_thread_create such as *new. At present *new is not
accessed after the thread has started but the implementation could
change. So new patch:
Index: subversion/svnserve/svnserve.c
===================================================================
--- subversion/svnserve/svnserve.c (revision 1480608)
+++ subversion/svnserve/svnserve.c (working copy)
@@ -455,6 +455,7 @@ int main(int argc, const char *argv[])
#if APR_HAS_THREADS
apr_threadattr_t *tattr;
apr_thread_t *tid;
+ apr_pool_t *iterpool;
struct serve_thread_t *thread_data;
#endif
@@ -973,6 +974,7 @@ int main(int argc, const char *argv[])
{
#if APR_HAS_THREADS
settings.single_threaded = FALSE;
+ iterpool = svn_pool_create(pool);
#else
/* No requests will be processed at all
* (see "switch (handling_mode)" code further down).
@@ -1093,7 +1095,7 @@ int main(int argc, const char *argv[])
particularly sophisticated strategy for a threaded server, it's
little different from forking one process per connection. */
#if APR_HAS_THREADS
- status = apr_threadattr_create(&tattr, connection_pool);
+ status = apr_threadattr_create(&tattr, iterpool);
if (status)
{
err = svn_error_wrap_apr(status, _("Can't create threadattr"));
@@ -1114,7 +1116,7 @@ int main(int argc, const char *argv[])
thread_data->params = ¶ms;
thread_data->pool = connection_pool;
status = apr_thread_create(&tid, tattr, serve_thread, thread_data,
- connection_pool);
+ iterpool);
if (status)
{
err = svn_error_wrap_apr(status, _("Can't create thread"));
@@ -1122,6 +1124,7 @@ int main(int argc, const char *argv[])
svn_error_clear(err);
exit(1);
}
+ svn_pool_clear(iterpool);
#endif
break;
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> On Wed, May 8, 2013 at 1:00 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>
>> The fix resolves the symptoms, but I would guess it might be better
>> to make the thread create (and destroy) its own pool, while using
>> some proper iterpool in the function that creates the pools. Handing
>> a temporary pool to a new thread doesn't give us clean and easy to
>> maintain code... as this old bug shows us.
>>
> It would not for two reasons:
> 1. We allocate connection stream in connection_pool in main thread
> before creating worker thread
> 2. Current APR implementation requires that apr_thread_t * is
> available during thread lifetime. And we don't have any kind of
> notification when thread completes.
We could do this:
Index: subversion/svnserve/svnserve.c
===================================================================
--- subversion/svnserve/svnserve.c (revision 1480565)
+++ subversion/svnserve/svnserve.c (working copy)
@@ -444,6 +444,7 @@ int main(int argc, const char *argv[])
apr_sockaddr_t *sa;
apr_pool_t *pool;
apr_pool_t *connection_pool;
+ apr_pool_t *iterpool;
svn_error_t *err;
apr_getopt_t *os;
int opt;
@@ -973,6 +974,7 @@ int main(int argc, const char *argv[])
{
#if APR_HAS_THREADS
settings.single_threaded = FALSE;
+ iterpool = svn_pool_create(pool);
#else
/* No requests will be processed at all
* (see "switch (handling_mode)" code further down).
@@ -1093,7 +1095,7 @@ int main(int argc, const char *argv[])
particularly sophisticated strategy for a threaded server, it's
little different from forking one process per connection. */
#if APR_HAS_THREADS
- status = apr_threadattr_create(&tattr, connection_pool);
+ status = apr_threadattr_create(&tattr, iterpool);
if (status)
{
err = svn_error_wrap_apr(status, _("Can't create threadattr"));
@@ -1122,6 +1124,7 @@ int main(int argc, const char *argv[])
svn_error_clear(err);
exit(1);
}
+ svn_pool_clear(iterpool);
#endif
break;
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, May 8, 2013 at 1:00 PM, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: woensdag 8 mei 2013 01:46
>> To: Subversion Development
>> Subject: Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for
>> testing/signing)
>>
>> On Sun, May 5, 2013 at 8:27 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> > On Sun, May 5, 2013 at 6:36 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <iv...@visualsvn.com>
>> wrote:
>> >>> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <be...@reser.org> wrote:
>> >>>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
>> >>>> fetch the proposed tarballs from here:
>> >>>> https://dist.apache.org/repos/dist/dev/subversion
>> >>>>
>> >>> I've got crash in svnserve when running 32-bit test suite over svn://
>> >>> protocol on Windows 7 (x64).
>> >> [..]
>> >>>
>> >>> I'm running test suite again to check if this issue has stable reproduction.
>> >>>
>> >> The crash happened again.
>> >>
>> >> It seems problem in apr_thread_* implementation and svnserve pool
>> >> management. Problem that we allocate apr_thread_t and
>> apr_threadattr_t
>> >> in connection_pool which is destroyed when created thread completes.
>> >> This lead access to freed memory in apr_thread_create() when
>> >> connection thread completes very fast. See apr_thread_create()
>> >> implementation on Windows (apr\threadproc\win32\thread.c:82)
>> > Attached patch fixes problem in my configuration.
>> >
>> Filed as issue 4365:
>> http://subversion.tigris.org/issues/show_bug.cgi?id=4365
>>
>> It's definitely 1.8.0-rc1 show stopper. We have to decide if proposed
>> workaround (allocate apr_threadattr_t in subpool) is enough fix for
>> 1.8.x
>
> The fix resolves the symptoms, but I would guess it might be better to make the thread create (and destroy) its own pool, while using some proper iterpool in the function that creates the pools. Handing a temporary pool to a new thread doesn't give us clean and easy to maintain code... as this old bug shows us.
>
It would not for two reasons:
1. We allocate connection stream in connection_pool in main thread
before creating worker thread
2. Current APR implementation requires that apr_thread_t * is
available during thread lifetime. And we don't have any kind of
notification when thread completes.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
RE: Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: woensdag 8 mei 2013 01:46
> To: Subversion Development
> Subject: Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for
> testing/signing)
>
> On Sun, May 5, 2013 at 8:27 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> > On Sun, May 5, 2013 at 6:36 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >>> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <be...@reser.org> wrote:
> >>>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
> >>>> fetch the proposed tarballs from here:
> >>>> https://dist.apache.org/repos/dist/dev/subversion
> >>>>
> >>> I've got crash in svnserve when running 32-bit test suite over svn://
> >>> protocol on Windows 7 (x64).
> >> [..]
> >>>
> >>> I'm running test suite again to check if this issue has stable reproduction.
> >>>
> >> The crash happened again.
> >>
> >> It seems problem in apr_thread_* implementation and svnserve pool
> >> management. Problem that we allocate apr_thread_t and
> apr_threadattr_t
> >> in connection_pool which is destroyed when created thread completes.
> >> This lead access to freed memory in apr_thread_create() when
> >> connection thread completes very fast. See apr_thread_create()
> >> implementation on Windows (apr\threadproc\win32\thread.c:82)
> > Attached patch fixes problem in my configuration.
> >
> Filed as issue 4365:
> http://subversion.tigris.org/issues/show_bug.cgi?id=4365
>
> It's definitely 1.8.0-rc1 show stopper. We have to decide if proposed
> workaround (allocate apr_threadattr_t in subpool) is enough fix for
> 1.8.x
The fix resolves the symptoms, but I would guess it might be better to make the thread create (and destroy) its own pool, while using some proper iterpool in the function that creates the pools. Handing a temporary pool to a new thread doesn't give us clean and easy to maintain code... as this old bug shows us.
The bug is in 1.6.x and 1.7.x but there the pool allocator isn't destroyed, which makes apr 'just use' released data' of the allocator.
In 1.8 the allocator is destroyed with the pool, so the memory is returned to the os. And now we actually notice the problem.
Bert
>
> --
> Ivan Zhakov
> CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, May 5, 2013 at 8:27 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sun, May 5, 2013 at 6:36 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <be...@reser.org> wrote:
>>>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
>>>> fetch the proposed tarballs from here:
>>>> https://dist.apache.org/repos/dist/dev/subversion
>>>>
>>> I've got crash in svnserve when running 32-bit test suite over svn://
>>> protocol on Windows 7 (x64).
>> [..]
>>>
>>> I'm running test suite again to check if this issue has stable reproduction.
>>>
>> The crash happened again.
>>
>> It seems problem in apr_thread_* implementation and svnserve pool
>> management. Problem that we allocate apr_thread_t and apr_threadattr_t
>> in connection_pool which is destroyed when created thread completes.
>> This lead access to freed memory in apr_thread_create() when
>> connection thread completes very fast. See apr_thread_create()
>> implementation on Windows (apr\threadproc\win32\thread.c:82)
> Attached patch fixes problem in my configuration.
>
Filed as issue 4365:
http://subversion.tigris.org/issues/show_bug.cgi?id=4365
It's definitely 1.8.0-rc1 show stopper. We have to decide if proposed
workaround (allocate apr_threadattr_t in subpool) is enough fix for
1.8.x
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, May 5, 2013 at 6:36 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <be...@reser.org> wrote:
>>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
>>> fetch the proposed tarballs from here:
>>> https://dist.apache.org/repos/dist/dev/subversion
>>>
>> I've got crash in svnserve when running 32-bit test suite over svn://
>> protocol on Windows 7 (x64).
> [..]
>>
>> I'm running test suite again to check if this issue has stable reproduction.
>>
> The crash happened again.
>
> It seems problem in apr_thread_* implementation and svnserve pool
> management. Problem that we allocate apr_thread_t and apr_threadattr_t
> in connection_pool which is destroyed when created thread completes.
> This lead access to freed memory in apr_thread_create() when
> connection thread completes very fast. See apr_thread_create()
> implementation on Windows (apr\threadproc\win32\thread.c:82)
Attached patch fixes problem in my configuration.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com