I have not read this patch carefully so I have no idea that the patch
solved the problem or not. I just have two question:
1) do we find the cause of the crash? (how to trigger the crash, in what
situation)
2) is it worth using smart pointer to prevent the crash? (maintain the
design principle of the ATS)
If we can not find a perfect solution now， why not just put it aside.
BTW: In taobao`s tree (base on 3.0.1 and some patches of our own), we
have not seen the crash for a long time.
On Tue, 2012-03-13 at 12:36 -0500, Alan M. Carroll wrote:
> I have a patch submitted for TS-857 which attempts to make a start on finer grained locking.
The essence of it is
>
> 1) Provide more powerful and standards compliant reference counted smart pointers - lib/ts/IntrusivePtr.h
>
> 2) Provide reference counted lists that are compatible with both (1) and existing intrusive
list support.
>
> The current IntrusivePtr.h does more than this but that is an artifact of the development
process, as I will describe later.
>
> For TS-857 in particular, the change is to replace the existing NetHandler open_list
with a reference counted list which uses the same internal links, and to change HttpServerSession
to use reference counted smart pointers. These changes mean that NetHandler can detect when
a VC is shared and avoid closing it underneath the other thread. The current Handle and callback
mechanism is discarded because it's a hack.
>
> Unfortunately, for reasons I don't understand, I can no longer replicate the problem
on my test setup, even going back to 3.1.1 where as before I could replicate it in under a
minute. So, while this patch seems stable I can not demonstrate that it is better with respect
to TS-857. It passes regression, my load testing, and does not seem to leak connections (traffic_line
reports a constant number of open connections even after much load testing).
>
> Reference counting is implemented only for UnixNetVConnections as the NetHandler open
list contains only instances of that type. Unfortunately the actual mechanism must be introduced
in to the class tree at NetVConnection because that is what HttpServerSession uses. The normal
reference counting cleanup is disabled at this point because it's either handled differently
(UnixNetVConnection) or not wanted (all other NetVConnection subclasses).
>
> The additional types in IntrusivePtr.h that are not used were used during earlier work
but it seemed later that changing only open_list was the best approach. The other types were
left in as they seem potentially useful.
>
> The question going forward is, should we commit this patch? Any comments are appreciated.
For now, my recommendation is to commit and resolve TS-857 unless someone else can either
replicate it or provide field report of its occurrence.
>
|