Re: Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)

On Wed, May 8, 2013 at 1:00 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan_at_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 <ivan_at_visualsvn.com> wrote:
>> > On Sun, May 5, 2013 at 6:36 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>> >> On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <ivan_at_visualsvn.com>
>> wrote:
>> >>> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <ben_at_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