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.

Reply via email to