TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

The problem with all of these is that locking for the VConnections is done poorly, if at all, and closing a VConnection and de-allocating are conflated. Changing that is hard, though.

My current plan is related set of changes

1) Use a smart pointer (based on lib/ts/Ptr.h) instead of raw pointers, separating out do_io_close and free. The former can be done as it is now, but the latter only when the last pointer is dropped.

2) Write a new queue class that uses lib/ts/Vec.h to create an expanding circular buffer.

3) Use (2) to replace the current queue support for read_ready_list and write_ready_list. The problem this solves is that these currently use intrusive list pointers and that just won't work with (1). OTOH the instrusive lists do avoid allocations for elements in the list. The circular queue of (2) should give us the same level of speed with an acceptable amount of allocation (should be roughly constant per process, as eventually the queue will get big enough).

4) When pulling an element off the ready lists, lock it. If it can't be locked, put it on the back of the queue. This should not have a big impact because AFAICT the only reason the lock will fail is because it's being closed in another thread in which case rapid processing isn't important.

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

RE: specific, could you tell me which bug numbers are addressed by your
changes?

Thanx,
john

On Fri, Dec 9, 2011 at 9:38 AM, John Plevyak <jp...@acm.org> wrote:

>
> Could you be more specific about the problem (e.g. big numbers)?
>  VConnection locking works fine in all cases that I know of.  There are
> some issues with closing and deallocation with threading and the higher
> layers, but these could be solved in a number of ways.  Smart pointers is
> one (and probably not a bad one) although it isn't a panacea, just a
> mechanism.  Similarly, a circular buffer queue is just a different way of
> viewing the data structure.  Sure, it has some advantages, particularly in
> exposure to deallocation and locking, but it isn't a silver bullet either.
>  I'd like to understand the core problem.
>
> john
>
>
> On Tue, Dec 6, 2011 at 12:22 PM, Alan M. Carroll <
> amc@network-geographics.com> wrote:
>
>> The problem with all of these is that locking for the VConnections is
>> done poorly, if at all, and closing a VConnection and de-allocating are
>> conflated. Changing that is hard, though.
>>
>> My current plan is related set of changes
>>
>> 1) Use a smart pointer (based on lib/ts/Ptr.h) instead of raw pointers,
>> separating out do_io_close and free. The former can be done as it is now,
>> but the latter only when the last pointer is dropped.
>>
>> 2) Write a new queue class that uses lib/ts/Vec.h to create an expanding
>> circular buffer.
>>
>> 3) Use (2) to replace the current queue support for read_ready_list and
>> write_ready_list. The problem this solves is that these currently use
>> intrusive list pointers and that just won't work with (1). OTOH the
>> instrusive lists do avoid allocations for elements in the list. The
>> circular queue of (2) should give us the same level of speed with an
>> acceptable amount of allocation (should be roughly constant per process, as
>> eventually the queue will get big enough).
>>
>> 4) When pulling an element off the ready lists, lock it. If it can't be
>> locked, put it on the back of the queue. This should not have a big impact
>> because AFAICT the only reason the lock will fail is because it's being
>> closed in another thread in which case rapid processing isn't important.
>>
>>
>

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Oh, I think it was there, but *someone* of fell intent removed it. Hmmmm. Unfortunately, the list of suspects is too long to put in this message.

http://people.apache.org/~amc/images/TS-Lock-Classes.png

Monday, December 12, 2011, 10:43:17 AM, you wrote:
>> I have
>> attached a diagram of this call chain, derived directly from an
>> actual stack trace.

> You you haven't.

Re: TS-857, TS-934, TS-1031

[Igor Galić <i....@brainsware.org>]

----- Original Message -----
> I want to express my regrets for not behaving in a professional
> manner as well. Let's move forward. I'll need some time to consider

Thank you very much, Alan and John.
I'm glad we're all back on the same page.

> John's proposal. I will note that I think we would do well for
> future participants to document clearly whatever locking structure
> emerges. I volunteer to do a Visio / UML diagram as part of that.

+1 - we badly need some documentation on how the core works
(and how to hack it)


i

-- 
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515  2EA5 4B1D 9E08 A097 C9AE

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

I want to express my regrets for not behaving in a professional manner as well. Let's move forward. I'll need some time to consider John's proposal. I will note that I think we would do well for future participants to document clearly whatever locking structure emerges. I volunteer to do a Visio / UML diagram as part of that.

Re: TS-857, TS-934, TS-1031

[Igor Galić <i....@brainsware.org>]

----- Original Message -----
> I have been thinking about this a lot and I think Alan is right.
>   The
> locking design of ATS is not appropriate
> for an Apache project.   The design worked when it was a few full
> time
> people sitting within feet of each
> other all day (and chunks of the night) but the world has moved on
> and we
> should move to a locking and threading
> model which is more robust and fits with the expectations of a larger
> fraction of the community.
> 
> I want to apologize for wrapping you all into my long running
> frustration
> with trying to keep this brittle system stable
> and open up discussion on how we can make it more stable, robust and
> easier
> to develop in.
> 
> It is abundantly clear that if anyone has to go to the lengths that
> Alan
> has been forced to to try to make this system
> work under load that it is the systems fault.
> 
> So, here is my proposal.
> 
> The old locking system was based on TryLocks which could not be taken
> forceably.  Moreover it depended on
> very subtle knowledge of which bits of the various data structures
> where
> protected by which locks.  This is clearly
> not sustainable, nor is it necessary any longer.   Modern threading
> systems
> work well with larger numbers of threads
> and fine grain locking.
> 
> So, let's change to the more conventional model with fine grained
> locks
> which protect data structures which are
> clearly labeled with the lock that protects them and have external
> APIs
> which enforce that protection.  These
> locks will be just taken in the standard manner, and we will have to
> ensure
> that the data structures are
> sliced so as to minimize lock contention in the standard manner.
> 
> Let's also have a "Transaction" object (essentially our current Mutex
> with
> additional tracking and book keeping)
> and an explicit mechanism for associating resources owned by
> Processors
> (e.g. NetVC) with a Transaction and for passing
> resources (e.g. a NetVC) from one Transaction to another and for
> returning
> the resource to the Processor when it is no longer required
> (close/free/release).  We can also use proxy smart pointers and
> encapsulation in debug mode to test that the ownership rules are
> being
> obeyed correctly and that "stale" pointers are not being accessed
> (i.e.
> after the resource has been released).
> 
> I believe that the problems we are currently seeing turn on an even
> more
> subtle issue when the ownership of a NetVC
> is passed from one transaction to another via the session manager.
> Getting that code to work stably required many
> a careful negotiation and resulted in something which is clearly very
> brittle and not maintainable.
> 
> I hope that we work through this and end up with a system which is
> substantially more maintainable easier to develop in.

+1

> Thanx
> john
> 
> 
> On Tue, Dec 13, 2011 at 10:30 PM, Alan M. Carroll <
> amc@network-geographics.com> wrote:
> 
> > Tuesday, December 13, 2011, 7:00:42 PM, you wrote:
> >
> > >> > No other thread can call vc->do_io_close if they don't have
> > >> > the
> > pointer
> > >> > to it.
> > >> Turns out that at least other thread does have a pointer.
> > > Then they should not.
> >
> > Great. Now explain it to the compiler. Let me know when you've done
> > that,
> > I'll be going back to work on IPv6.
> >
> > > If you are really having a problem with this I am going to have
> > > to go
> > back
> > > through your checkins and see what changes might have been
> > > motivated by
> > > such a fundamental lack of understanding of parallel programming.
> > >  This
> > is
> > > very worrying.
> >
> > I think if you are worried, you should definitely go back and
> > check.
> >
> >
> >
> 

-- 
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515  2EA5 4B1D 9E08 A097 C9AE

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

It would be a serious undertaking, but it is clearly necessary and I think
it can be done incrementally... that is we can move to the new model within
the Processors and then transition the APIs.  Based on that, in the short
term we can take a more heavy handed approach to solving this problem and
just wrap some big locks around the problem areas then push them down (a la
multi-threading the FreeBSD kernel).   In the short term we will have some
contention problems, but that is better than crashing.

john

On Wed, Dec 14, 2011 at 9:52 AM, Leif Hedstrom <zw...@apache.org> wrote:

> On 12/14/11 10:28 AM, John Plevyak wrote:
>
>> The old locking system was based on TryLocks which could not be taken
>> forceably.  Moreover it depended on
>> very subtle knowledge of which bits of the various data structures where
>> protected by which locks.  This is clearly
>> not sustainable, nor is it necessary any longer.   Modern threading
>> systems
>> work well with larger numbers of threads
>> and fine grain locking.
>>
>> So, let's change to the more conventional model with fine grained locks
>> which protect data structures which are
>> clearly labeled with the lock that protects them and have external APIs
>> which enforce that protection.  These
>> locks will be just taken in the standard manner, and we will have to
>> ensure
>> that the data structures are
>> sliced so as to minimize lock contention in the standard manner.
>>
>> Let's also have a "Transaction" object (essentially our current Mutex with
>> additional tracking and book keeping)
>> and an explicit mechanism for associating resources owned by Processors
>> (e.g. NetVC) with a Transaction and for passing
>> resources (e.g. a NetVC) from one Transaction to another and for returning
>> the resource to the Processor when it is no longer required
>> (close/free/release).  We can also use proxy smart pointers and
>> encapsulation in debug mode to test that the ownership rules are being
>> obeyed correctly and that "stale" pointers are not being accessed (i.e.
>> after the resource has been released).
>>
>
> This sounds like a great idea! How do we get there? This would be a pretty
> significant undertaking, and touches pretty much all code. So, perhaps this
> is a target goal for v4.0? It sounds like we might need to try to have a
> "hackathon" event again somewhere? :)
>
>
>> I believe that the problems we are currently seeing turn on an even more
>> subtle issue when the ownership of a NetVC
>> is passed from one transaction to another via the session manager.
>> Getting that code to work stably required many
>> a careful negotiation and resulted in something which is clearly very
>> brittle and not maintainable.
>>
>
> Hopefully we can figure it out to at least make it not crash in the short
> term? I could quite possible be at fault here too, I added extra complexity
> by adding an (optional) per thread session pool. I hope that is not causing
> some of these problems?
>
> Cheers,
>
> -- leif
>

Re: TS-857, TS-934, TS-1031

[Leif Hedstrom <zw...@apache.org>]

On 12/14/11 10:28 AM, John Plevyak wrote:
> The old locking system was based on TryLocks which could not be taken
> forceably.  Moreover it depended on
> very subtle knowledge of which bits of the various data structures where
> protected by which locks.  This is clearly
> not sustainable, nor is it necessary any longer.   Modern threading systems
> work well with larger numbers of threads
> and fine grain locking.
>
> So, let's change to the more conventional model with fine grained locks
> which protect data structures which are
> clearly labeled with the lock that protects them and have external APIs
> which enforce that protection.  These
> locks will be just taken in the standard manner, and we will have to ensure
> that the data structures are
> sliced so as to minimize lock contention in the standard manner.
>
> Let's also have a "Transaction" object (essentially our current Mutex with
> additional tracking and book keeping)
> and an explicit mechanism for associating resources owned by Processors
> (e.g. NetVC) with a Transaction and for passing
> resources (e.g. a NetVC) from one Transaction to another and for returning
> the resource to the Processor when it is no longer required
> (close/free/release).  We can also use proxy smart pointers and
> encapsulation in debug mode to test that the ownership rules are being
> obeyed correctly and that "stale" pointers are not being accessed (i.e.
> after the resource has been released).

This sounds like a great idea! How do we get there? This would be a pretty 
significant undertaking, and touches pretty much all code. So, perhaps this 
is a target goal for v4.0? It sounds like we might need to try to have a 
"hackathon" event again somewhere? :)
>
> I believe that the problems we are currently seeing turn on an even more
> subtle issue when the ownership of a NetVC
> is passed from one transaction to another via the session manager.
> Getting that code to work stably required many
> a careful negotiation and resulted in something which is clearly very
> brittle and not maintainable.

Hopefully we can figure it out to at least make it not crash in the short 
term? I could quite possible be at fault here too, I added extra complexity 
by adding an (optional) per thread session pool. I hope that is not causing 
some of these problems?

Cheers,

-- leif

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

I have been thinking about this a lot and I think Alan is right.   The
locking design of ATS is not appropriate
for an Apache project.   The design worked when it was a few full time
people sitting within feet of each
other all day (and chunks of the night) but the world has moved on and we
should move to a locking and threading
model which is more robust and fits with the expectations of a larger
fraction of the community.

I want to apologize for wrapping you all into my long running frustration
with trying to keep this brittle system stable
and open up discussion on how we can make it more stable, robust and easier
to develop in.

It is abundantly clear that if anyone has to go to the lengths that Alan
has been forced to to try to make this system
work under load that it is the systems fault.

So, here is my proposal.

The old locking system was based on TryLocks which could not be taken
forceably.  Moreover it depended on
very subtle knowledge of which bits of the various data structures where
protected by which locks.  This is clearly
not sustainable, nor is it necessary any longer.   Modern threading systems
work well with larger numbers of threads
and fine grain locking.

So, let's change to the more conventional model with fine grained locks
which protect data structures which are
clearly labeled with the lock that protects them and have external APIs
which enforce that protection.  These
locks will be just taken in the standard manner, and we will have to ensure
that the data structures are
sliced so as to minimize lock contention in the standard manner.

Let's also have a "Transaction" object (essentially our current Mutex with
additional tracking and book keeping)
and an explicit mechanism for associating resources owned by Processors
(e.g. NetVC) with a Transaction and for passing
resources (e.g. a NetVC) from one Transaction to another and for returning
the resource to the Processor when it is no longer required
(close/free/release).  We can also use proxy smart pointers and
encapsulation in debug mode to test that the ownership rules are being
obeyed correctly and that "stale" pointers are not being accessed (i.e.
after the resource has been released).

I believe that the problems we are currently seeing turn on an even more
subtle issue when the ownership of a NetVC
is passed from one transaction to another via the session manager.
Getting that code to work stably required many
a careful negotiation and resulted in something which is clearly very
brittle and not maintainable.

I hope that we work through this and end up with a system which is
substantially more maintainable easier to develop in.

Thanx
john


On Tue, Dec 13, 2011 at 10:30 PM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> Tuesday, December 13, 2011, 7:00:42 PM, you wrote:
>
> >> > No other thread can call vc->do_io_close if they don't have the
> pointer
> >> > to it.
> >> Turns out that at least other thread does have a pointer.
> > Then they should not.
>
> Great. Now explain it to the compiler. Let me know when you've done that,
> I'll be going back to work on IPv6.
>
> > If you are really having a problem with this I am going to have to go
> back
> > through your checkins and see what changes might have been motivated by
> > such a fundamental lack of understanding of parallel programming.  This
> is
> > very worrying.
>
> I think if you are worried, you should definitely go back and check.
>
>
>

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Tuesday, December 13, 2011, 7:00:42 PM, you wrote:

>> > No other thread can call vc->do_io_close if they don't have the pointer
>> > to it.
>> Turns out that at least other thread does have a pointer.
> Then they should not.

Great. Now explain it to the compiler. Let me know when you've done that, I'll be going back to work on IPv6.

> If you are really having a problem with this I am going to have to go back
> through your checkins and see what changes might have been motivated by
> such a fundamental lack of understanding of parallel programming.  This is
> very worrying.

I think if you are worried, you should definitely go back and check.

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

On Tue, Dec 13, 2011 at 3:51 PM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> Tuesday, December 13, 2011, 3:30:06 PM, you wrote:
>
> > No other thread can call vc->do_io_close if they don't have the pointer
> to
> > it.
>

> Turns out that at least other thread does have a pointer.
>

Then they should not.


>
> > It is up to the protocol engine to ensure that all pointers are clear.
>
> Still not clear on how the protocol engine clears pointers in other
> threads.
>

All the pointers are covered by the same mutex.  Take the mutex.  Clear the
pointers.  That is how multi-threaded programming works.


>
> > That means that you need to ensure that even if there are two different
> > classes in two different threads that the pointers to the NetVC are
> stored
> > under the same instance of Mutex.
>
> That doesn't help, unless one of the threads holds the mutex the entire
> lifetime of the object. If at any time one (non-NetHandler) thread drops
> the mutex, then the other (NetHandler) thread can modify the VC to a state
> where using the pointer will cause a crash, such as by swizzling the VFPTR,
> or setting the nh member to zero (which ironically crashes on the mutex
> check in do_io_close), or even re-allocating it so the wrong connection is
> closed. Preventing simultaneous access isn't effective if single access
> processing can leave the target of the pointer in an undetectably bad state.
>

The pointer is only swizzled AFTER do_io_close is called.   You have to
clear all other pointers to the object BEFORE calling do_io_close.

1. Create a Mutex.
2. only access pointers to NetVC under mutex
3. Release Mutex
4. Do what you want but DO NOT call do_io_close
5. Take out Mutex
6. Clear all pointers to NetVC (except the one you are holding)
7. Call do_io_close and clear that pointer.
8. DONE, safely, just as it has worked since 1996.


>
> > So you clear all pointers except
> > the last one, call vc->do_io_closed() then clear the last via vc = NULL
> and
> > you are done.
>
> Again, the problem is the implementation mechanism by which those pointers
> are cleared. If only there were some technique were you could, say, keep a
> count of the number of outstanding pointers and only call do_io_close when
> the last one was cleared.
>

The concept is called "understanding ownership".  The NetVC is not some
shared thing.  It is a thing that should have one single owner.   That
owner should know where all the pointers to the object are.  That owner
should not invalidate the object while there are outstanding pointers.  I
know you have written code in C.   This is just like that.  You MUST clear
all pointers in C before calling free() otherwise when you access through
the pointer you will be touching free'd memory.  This is the exact same
thing.  Exactly the same as any program written with pointers. It is no
harder.  Yes, it is multi-threaded, but it isn't rocket science.  You just
have to have exclusive access to any shared state.  That is how parallel
programming works.  I don't know how much clearer I can be.

If you are really having a problem with this I am going to have to go back
through your checkins and see what changes might have been motivated by
such a fundamental lack of understanding of parallel programming.  This is
very worrying.


>
> > This is multi-threaded programming 101.  If you can't handle this then
> you
> > can't write multi-threaded code.
>
> I'll just re-assign all my bugs to you and zwoop, then, since I appear to
> lack even basic programming knowledge.
>

You really don't get the concept of exclusive access to shared state and
clearing pointers before invalidating an object???? I just can't believe
that.  We must have some sort of misunderstanding.


>
> > It sounds like there is a bug in the way that the HTTP sessions are
> > handled.  I need to look at the stack traces and see if I can figure out
> > where the bug is.  I you have some way to reproduce the problem that
> would
> > be great too.
>
> Set up forward transparent proxy and drive traffic through it. Prior to
> 3.1.1, we would get a crash every few hours. bcall and weijin have seen the
> same crash.
>
> > Smart pointers are a way of hiding the bugs, not a solution.
>
> Some one should let the C++ Standards Committee know.
>

As I have very clearly stated before, shard_ptr (Ptr) pointers are to be
used when there is no clear owner to an object, e.g. Mutex, IOBufferBlock
etc.  That is when the object is truely "shared".  The are not to be used
when there is an owner.   This is the case in Traffic Server and has been
since the beginning. It is also the case at my current employer who as many
thousands of programmers working very successfully with this convention.
 There are very few shared_ptr (Ptr) uses in TS or shared_ptr<> in the vast
amount of parallel code have worked with because of this.

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Tuesday, December 13, 2011, 3:30:06 PM, you wrote:

> No other thread can call vc->do_io_close if they don't have the pointer to
> it.

Turns out that at least other thread does have a pointer.

> It is up to the protocol engine to ensure that all pointers are clear.

Still not clear on how the protocol engine clears pointers in other threads.

> That means that you need to ensure that even if there are two different
> classes in two different threads that the pointers to the NetVC are stored
> under the same instance of Mutex.
 
That doesn't help, unless one of the threads holds the mutex the entire lifetime of the object. If at any time one (non-NetHandler) thread drops the mutex, then the other (NetHandler) thread can modify the VC to a state where using the pointer will cause a crash, such as by swizzling the VFPTR, or setting the nh member to zero (which ironically crashes on the mutex check in do_io_close), or even re-allocating it so the wrong connection is closed. Preventing simultaneous access isn't effective if single access processing can leave the target of the pointer in an undetectably bad state.
 
> So you clear all pointers except
> the last one, call vc->do_io_closed() then clear the last via vc = NULL and
> you are done.

Again, the problem is the implementation mechanism by which those pointers are cleared. If only there were some technique were you could, say, keep a count of the number of outstanding pointers and only call do_io_close when the last one was cleared.

> This is multi-threaded programming 101.  If you can't handle this then you
> can't write multi-threaded code.

I'll just re-assign all my bugs to you and zwoop, then, since I appear to lack even basic programming knowledge.

> It sounds like there is a bug in the way that the HTTP sessions are
> handled.  I need to look at the stack traces and see if I can figure out
> where the bug is.  I you have some way to reproduce the problem that would
> be great too.

Set up forward transparent proxy and drive traffic through it. Prior to 3.1.1, we would get a crash every few hours. bcall and weijin have seen the same crash.

> Smart pointers are a way of hiding the bugs, not a solution.

Some one should let the C++ Standards Committee know.
 

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

This worked for 15 years.  This is how Traffic Server was designed and how
it has run for years at AOL with uptime measured in months under very high
load on multi-processors with weaker memory guarantees than x86 (e.g.
Alpha).

Some new bug comes along and suddenly something which has been field tested
for over a decade is now "does not work".  Ha.


> > Clear all pointers to a NetVC then call vc->do_io_close.
>
> > Period.
>
> That does not work. Period. You can crash calling vc->do_io_close if
> another thread de-allocates it at the same time. I also don't see how you
> can clear all pointers when those pointer are in two different instances of
> two different classes in two different threads. Also, how do you call
> vc->do_io_close if you've cleared all the pointers?
>

No other thread can call vc->do_io_close if they don't have the pointer to
it.  It is up to the protocol engine to ensure that all pointers are clear.
 That means that you need to ensure that even if there are two different
classes in two different threads that the pointers to the NetVC are stored
under the same instance of Mutex.  That is just how multi-threaded
programming works.  A Mutex defines a region of memory which can be
accessed at only by one thread at a time.  So you clear all pointers except
the last one, call vc->do_io_closed() then clear the last via vc = NULL and
you are done.

This is multi-threaded programming 101.  If you can't handle this then you
can't write multi-threaded code.


>
> Perhaps if you could be more specific about to actually implement your
> recommendation, I might be able to see how to fix this problem.
>

It sounds like there is a bug in the way that the HTTP sessions are
handled.  I need to look at the stack traces and see if I can figure out
where the bug is.  I you have some way to reproduce the problem that would
be great too.

Smart pointers are a way of hiding the bugs, not a solution.  We don't want
hidden bugs and code accessing stale memory.   Buggy code should crash as
soon as possible so that the bug can be fixed and the system made stable.

john

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Tuesday, December 13, 2011, 1:00:11 PM, you wrote:

> Clear all pointers to a NetVC then call vc->do_io_close.

> Period.

That does not work. Period. You can crash calling vc->do_io_close if another thread de-allocates it at the same time. I also don't see how you can clear all pointers when those pointer are in two different instances of two different classes in two different threads. Also, how do you call vc->do_io_close if you've cleared all the pointers?

Perhaps if you could be more specific about to actually implement your recommendation, I might be able to see how to fix this problem.

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

This is done intentionally.  The reason is to catch bugs, not to hide them.
  By overwriting the pointer, bugs in client code will be caught faster and
caught in optimized code which is more likely to exhibit race conditions.
 It makes working with NetVCs much safer as it helps you find bugs in your
code where you are not clearing all the pointers to the NetVC before
calling close.

The core problem is that every crash will come out of the event, cache or
network subsystems, because they generate the callbacks and the bug is 99%
of the time in the client code, but client code writers like to blame the
messenger so they are constantly trying to patch the event, cache and net
system when the problem is theirs.

In this case the API is really simple.

Clear all pointers to a NetVC then call vc->do_io_close.

Period.

The problem is that someone is not doing that and then is shocked that the
NetVC disappears after close is called. Adding a lot of baggage to the
system and storing the NetVC pointers with smart pointers so that when
someone forgets to clear a pointer they can still access the stale memory
without crashing the system will result in subtle bugs.  There is a bug and
it needs to be fixed.  We should not be wallpapering over water damage.
Let's get to the root of the problem and fix it, not sweep it under the rug.


john


On Tue, Dec 13, 2011 at 8:10 AM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> Another point on this issue is why the NetVC allocation scheme swizzles
> the virtual function pointer, rather than having a separate pointer for the
> free list. This makes working with the NetVCs far more dangerous and I
> don't see what the benefit that justifies this risk is. Is there any
> possibility of changing this back to be in the same style as the intrusive
> linked list?
>
>

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Another point on this issue is why the NetVC allocation scheme swizzles the virtual function pointer, rather than having a separate pointer for the free list. This makes working with the NetVCs far more dangerous and I don't see what the benefit that justifies this risk is. Is there any possibility of changing this back to be in the same style as the intrusive linked list?

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Monday, December 12, 2011, 1:51:54 PM, you wrote:

> When I left Inktomi the inactivity timer was managed directly i.e.
> INACTIVITY_TIMEOUT was set.  It looks like someone checked in an
> InactivityCop.   This is protected by the NetHandler mutex.  It also takes
> out the NetVC mutex in case there is a timeout between the net_accept() and
> the immediate event where the NetVC is transferred to the controlling Net
> thread, but that can't happen as the inactivity_timeout_at is set to 0 in
> close_UnixNetVConnection() so this is strictly unnecessary (although I
> don't think harmful).  We could remove this.

I tracked down several crashes to InactivityCop. Because the crashes frequently left the process state very mangled, I can't be sure of how involved it was, but I suspect it of de-allocating NetVCs while they were in use.


> WTF: I have never in my life seen the code in
> NEtVConnection::Handler::do_locked_io_close().   I have never even seen a
> clas NetVConnection::Handler. This is extremely dangerous code. I have no
> idea what it is trying to do, but I would strongly suggest removing it and
> replacing it with netvc->do_io_close().

On the other had, after adding the NetVConnection::Handler change, my client observed almost an order of magnitude decrease in crashes related to this on a heavily loaded server. They are currently running a live site with this change.

Conversely, the diagram here

http://people.apache.org/~amc/images/TS-Lock-Classes.png

is a post mortem of a crash on the 3.0.X codebase where the call was to netvc->do_io_close() (on the right, "1: do_io_close"). This is the 3.0.X codebase, before I made any changes.

The crash happened because the VC was re-allocated while netvc->do_io_close was executing. At the point netvc->do_io_close is called in this situation, there is no way to guarantee that the pointer is still valid (because HttpServerSession stores a raw pointer to a netVC being handled by a different NetHandler). The mutex check you mention happens inside the method, by which time it is too late.

> It calls "delete" for gosh sakes!  This is a complete NONO.  Before this
> there were NO malloc's or free's on the critical path.  This code is both
> insanely inefficient as well as dangerous.

I was not happy with it either, although as noted in the comments, live testing indicates that it happens only every few hours even with tens of thousands of connections / second. That field testing is the reason I was proceeding on this tack. The changes I proposed that started this string was to do something better.

> do_io_close sets the "closed" flag which is then correctly managed by when
> the NetHandler mutex is being held by the NetHandler.

> The code in NetVConnection::Handle::do_locked_io_close is going to mangle
> the data structures!

Why? It handles the NetVC by calling netvc->do_io_close.

> And yes, I do mean that it
> should call do_io_close which DOES NOT delete the NetVC unless
nh->>mutex->thread_holding == this_ethread() (in other words when it is
> currently holding the NetHandler mutex).

Before I made any changes (when the code was operating as you outline) it would crash inside UnixNetVConnection::do_io_close, AFAICT for this reason. But it's clear that just calling the do_io_close method is not safe. The problem (it seems to me) is that the NetVC can be deleted by the thread holding the NH lock while another thread is invoking methods on the NetVC (such as do_io_close).


> Perhaps there is some confusion about what "close" means.

Quite likely, as it's not really mentioned anywhere that I have found.

> Do not cull entries from internal queues.  Just call do_io_close() and the
> owner of the memory and NetVC will reclaim it in good time.  Smart pointers
> of this type are a crutch.  Some companies (including the one I now work
> at) have an style guide which explicitly states that shared smart pointers
> should not be used except in extraordinary circumstances when ownership
> really is shared (e.g. Mutex).  In this case ownership is extremely clear.

There seems to be shared access between NetHandler, InactivityCop, and HttpServerSession.

> The NetVC is owned by the NetHandler, period.

How are the other two classes to safely access the NetVC from different threads? For a specific example, look at the diagram mentioned early and let me know how HttpServerSession should call netvc->do_io_close in a way that doesn't have the possibility that the virtual function pointer is garbage (due to the NetVC being on the free list) at the moment netvc->do_io_close is invoked. It can't look in the NetVC pointer to find the owning NetHandler, because that presumes the validity of the pointer.

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

On Mon, Dec 12, 2011 at 7:21 AM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> Friday, December 9, 2011, 5:17:56 PM, you wrote:
>
> > NetHandler::mainNetEvent doesn't lock the VCs because it doesn't need to
> > until it wants to access the internal data at which time it locks it.
> >  Therefore the internal data is protected by the NetVC mutex.  However,
> the
> > existance is NOT protected by that mutex.
>
> That's not what I have seen in debugging runs. i need to double check the
> actual call stack but I see calls down through read and in to close that
> don't lock the VC


There are 4 locks of interest: the NetHandler, the NetVc, the read VIO and
the write VIO, and they are used to protect different things.  There are
likely some missuses which are causing problems.

 The NetHandler lock is used to protect the data structures of the
NetHandler, which include the queues that the NetVC is in and the Link
fields for those queues in the NetVC, but not the other internal values of
the NetVC.  The NetVC lock is only used during connecting at the point
where the NetVC has not yet been handed over to the protocol SM.  The read
VIO and write VIO mutexes control the internal data structures for the
NetVC.  They are typically the same (although they could be different).  To
set the closed flag, a protocol engine needs to acquire both read and write
VIO mutexes and clear all pointers to the NetVC.  To delete the NetVC the
NetHandler mutex must be acquired and the 'closed' flag (previously set
after the protocol engine acquired both read and write VIO mutexes) must be
acquired and the NetVC removed from the NetVC queue.

read_from_net is called from the NetHandler under it's lock.  It takes out
the read VIO mutex.  It does NOT take out the NetVC mutex because that
mutex is solely used BEFORE the NET_EVENT_OPEN callback which passes the
NetVC to the protocol engine.

When I left Inktomi the inactivity timer was managed directly i.e.
INACTIVITY_TIMEOUT was set.  It looks like someone checked in an
InactivityCop.   This is protected by the NetHandler mutex.  It also takes
out the NetVC mutex in case there is a timeout between the net_accept() and
the immediate event where the NetVC is transferred to the controlling Net
thread, but that can't happen as the inactivity_timeout_at is set to 0 in
close_UnixNetVConnection() so this is strictly unnecessary (although I
don't think harmful).  We could remove this.


> .
>
> > The NetHandler lock is used to protect the allocation, deletion of the
> > NetVC.  The owner of the NetVC lock can set the 'closed' flag, but can't
> > delete the NetVC unless they own the NetHandler lock.
>
> Sure it can, it just calls do_io_close. I direct your attention to
> HttpServerSession.cc:do_io_close which does precisely this. I have attached
> a diagram of this call chain, derived directly from an actual stack trace.
>

WTF: I have never in my life seen the code in
NEtVConnection::Handler::do_locked_io_close().   I have never even seen a
clas NetVConnection::Handler. This is extremely dangerous code. I have no
idea what it is trying to do, but I would strongly suggest removing it and
replacing it with netvc->do_io_close().

It calls "delete" for gosh sakes!  This is a complete NONO.  Before this
there were NO malloc's or free's on the critical path.  This code is both
insanely inefficient as well as dangerous.

do_io_close sets the "closed" flag which is then correctly managed by when
the NetHandler mutex is being held by the NetHandler.

The code in NetVConnection::Handle::do_locked_io_close is going to mangle
the data structures!



>
> > A well functioning
> > protocol engine would hold the NetVC mutex,. remove all references to the
> > NetVC  then call do_io_close().
>
> But isn't it the NetHandler mutex that controls de-allocation? Holding
> just the NetVC mutex and calling do_io_close doesn't follow that protocol.
>

Sorry, I meant the read and write VIO mutexes.   And yes, I do mean that it
should call do_io_close which DOES NOT delete the NetVC unless
nh->mutex->thread_holding == this_ethread() (in other words when it is
currently holding the NetHandler mutex).

do_io_close() does the right thing. It sets a flag "closed" and then *if*
it holds the NetHandler mutex it removes the NetVC from the queues etc.


>
> > It looks like there are crashes because someone is calling do_io_close
> > without ensuring that all the holders of pointers to the NetVC have been
> > cleared.  Once do_io_close() is called, the NetVC is dead, it can't be
> > accessed any more because the NetHandler can access the "closed" flag
> > WITHOUT the NetVC lock, but WITH the NetHandler lock and delete the
> NetVC.
>
> But how is that to be done? In the case I attached, the NetVC is possibly
> present in a ready queue of a NetHandler which is different from the NH
> that is invoking do_io_close. How can it find the correct NH and remove the
> NetVC from those queues? One must also be concerned about deadlock, where
> two threads try to close the NetVC, each holding the NetHandler mutex the
> other thread wants.
>

You don't!!!! you just set the "closed" flag.  The do_io_close() code
checks that the current thread is holding the NetHandler for the mutex of
the target VC.  Therefore it is safe under these circumstances.  You do not
want to try to delete the NetVC.  Just call do_io_close and it will do the
right thing.  Trying to forceably delete something between threads with
shared state is a really really bad idea.

Perhaps there is some confusion about what "close" means.  In this context
it means close the VC which only indirectly refers to the underlying socket
because this happens to be a NetVC. It is a single to the controlling
processor, in this case represented as the NetHandler that the VC is no
longer referenced by the protocol engine and should be reclaimed.  It very
much does not mean to close the underlying socket immediately or to reclaim
the underlying memory immediately.


>
> > There are a variety of ways of throwing smart pointers at the "problem"
> > which basically confuse when the object is actually live and let people
> > continue to hold pointers to dead/closed objects in buggy code.
>
> No, it's using them to help distinguish between "live" and "de-allocated".
> It seems to me to be the least costly / complex way for thread 1 to in
> effect cull entries from thread 2's internal queues. With smart pointers
> thread 1 doesn't have to know which thread is thread 2, or even if it
> exists. If you have an alternate coordination mechanism, please suggest it.
>

Do not cull entries from internal queues.  Just call do_io_close() and the
owner of the memory and NetVC will reclaim it in good time.  Smart pointers
of this type are a crutch.  Some companies (including the one I now work
at) have an style guide which explicitly states that shared smart pointers
should not be used except in extraordinary circumstances when ownership
really is shared (e.g. Mutex).  In this case ownership is extremely clear.
 The NetVC is owned by the NetHandler, period.   Only the NetHandler (or by
extension the holder of the NetHandler lock) has any right to the
NetHandler queues and can reclaim the NetVC memory.

This is most definitely a situation where a smart pointer is should never
be used.  Mutex, MIOBuffer, these are true shared objects which have no one
true owner.  These are the objects which deserve and use smart pointers.
 Only a handful of objects in ATS are reference counted, and all of them
are like this: objects with no single owner.


> > This doesn't change the fact that if they made sure not to hold stale
> > pointers, there wouldn't be a problem in the first place.
>
> Yes, but the question is, how can a thread make sure to not hold a stale
> pointer to an object which can be closed / de-allocated in another thread?
> Smart pointers make that simple without deadlock and with minimal lock
> contention.
>


The answer is very very simple.  Remove all references and call do_io_close
which does the right thing.  It checks to see if it already has the
NetHandler lock, and if so it can reclaim the NetVC, and if not it sets the
"closed" flag for later handling under the NetHandler mutex.

Re: TS-857, TS-934, TS-1031

[Igor Galić <i....@brainsware.org>]

> > The NetHandler lock is used to protect the allocation, deletion of
> > the
> > NetVC.  The owner of the NetVC lock can set the 'closed' flag, but
> > can't
> > delete the NetVC unless they own the NetHandler lock.
> 
> Sure it can, it just calls do_io_close. I direct your attention to
> HttpServerSession.cc:do_io_close which does precisely this. I have
> attached a diagram of this call chain, derived directly from an
> actual stack trace.

You you haven't.

i

-- 
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515  2EA5 4B1D 9E08 A097 C9AE

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

Friday, December 9, 2011, 5:17:56 PM, you wrote:

> NetHandler::mainNetEvent doesn't lock the VCs because it doesn't need to
> until it wants to access the internal data at which time it locks it.
>  Therefore the internal data is protected by the NetVC mutex.  However, the
> existance is NOT protected by that mutex.

That's not what I have seen in debugging runs. i need to double check the actual call stack but I see calls down through read and in to close that don't lock the VC.

> The NetHandler lock is used to protect the allocation, deletion of the
> NetVC.  The owner of the NetVC lock can set the 'closed' flag, but can't
> delete the NetVC unless they own the NetHandler lock.

Sure it can, it just calls do_io_close. I direct your attention to HttpServerSession.cc:do_io_close which does precisely this. I have attached a diagram of this call chain, derived directly from an actual stack trace.
 
> A well functioning
> protocol engine would hold the NetVC mutex,. remove all references to the
> NetVC  then call do_io_close().

But isn't it the NetHandler mutex that controls de-allocation? Holding just the NetVC mutex and calling do_io_close doesn't follow that protocol.

> It looks like there are crashes because someone is calling do_io_close
> without ensuring that all the holders of pointers to the NetVC have been
> cleared.  Once do_io_close() is called, the NetVC is dead, it can't be
> accessed any more because the NetHandler can access the "closed" flag
> WITHOUT the NetVC lock, but WITH the NetHandler lock and delete the NetVC.

But how is that to be done? In the case I attached, the NetVC is possibly present in a ready queue of a NetHandler which is different from the NH that is invoking do_io_close. How can it find the correct NH and remove the NetVC from those queues? One must also be concerned about deadlock, where two threads try to close the NetVC, each holding the NetHandler mutex the other thread wants.

> There are a variety of ways of throwing smart pointers at the "problem"
> which basically confuse when the object is actually live and let people
> continue to hold pointers to dead/closed objects in buggy code.

No, it's using them to help distinguish between "live" and "de-allocated". It seems to me to be the least costly / complex way for thread 1 to in effect cull entries from thread 2's internal queues. With smart pointers thread 1 doesn't have to know which thread is thread 2, or even if it exists. If you have an alternate coordination mechanism, please suggest it.

> This doesn't change the fact that if they made sure not to hold stale
> pointers, there wouldn't be a problem in the first place.
 
Yes, but the question is, how can a thread make sure to not hold a stale pointer to an object which can be closed / de-allocated in another thread? Smart pointers make that simple without deadlock and with minimal lock contention.
 

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

Clearly it should never be the case that the NetVC is deallocated while
still 'in use'.

NetHandler::mainNetEvent doesn't lock the VCs because it doesn't need to
until it wants to access the internal data at which time it locks it.
 Therefore the internal data is protected by the NetVC mutex.  However, the
existance is NOT protected by that mutex.

The NetHandler lock is used to protect the allocation, deletion of the
NetVC.  The owner of the NetVC lock can set the 'closed' flag, but can't
delete the NetVC unless they own the NetHandler lock.   A well functioning
protocol engine would hold the NetVC mutex,. remove all references to the
NetVC  then call do_io_close().

It looks like there are crashes because someone is calling do_io_close
without ensuring that all the holders of pointers to the NetVC have been
cleared.  Once do_io_close() is called, the NetVC is dead, it can't be
accessed any more because the NetHandler can access the "closed" flag
WITHOUT the NetVC lock, but WITH the NetHandler lock and delete the NetVC.

There are a variety of ways of throwing smart pointers at the "problem"
which basically confuse when the object is actually live and let people
continue to hold pointers to dead/closed objects in buggy code.

This doesn't change the fact that if they made sure not to hold stale
pointers, there wouldn't be a problem in the first place.   The only way a
NetVC is deleted is when it is closed, which is done by a protocol engine
which should be in charge of all pointers to the NetVC and should be
managing them.

Nobody should be holding a pointer to a NetVC after do_io_close() is
called.  That is the core problem.

john

On Fri, Dec 9, 2011 at 12:26 PM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> The core problem is that VC connection locking does not work. VCs are
> deallocated while still in use which leads to crashes. In fact, the general
> handling of VCs in NetHandler::mainNetEvent doesn't lock the VCs at all, so
> the cross thread access can't be safe. Moreover, because all the VC
> references are stored as raw pointers, there's no good way to safely access
> them across threads or even detect deallocation. I tried putting in
> generation numbers but that was insufficient (you can still get crashes
> because the virtual function pointer has been swizzled by the deallocator
> before you even get to the VC). All the crashes in the listed bugs stem
> from VC locking failures leading to cross thread corruption.
>
> I've been at quite a loss on how to fix this.
>
> The two root causes, IMHO, are
>
> 1) VCs are ephemeral and stored purely by pointer, which means the
> accessor cannot have thread safe access. You can use pointers if your
> objects don't evaporate asynchronously. The use of raw pointers also means
> there's no way to detect dangling references.
>
> 2) Closing and deallocation are conflated, so that when a VC needs to be
> closed, it is also de-allocated. There are good reasons to get the close
> done ASAP but the de-allocation is not nearly a time critical.
>
> I am looking at using smart pointers to alleviate these two issues.
> However, that creates problems because the current queue mechanisms require
> the use of raw pointers. AFAICT the point of these is to minimize
> allocation on the fast path. The circular buffer is an attempt to have as
> little allocation impact as possible, otherwise I would just use a standard
> allocate per element style.
>
> Experimentally, the fix I did for TS-934 had a strong effect on reducing
> the problem, although it was not a complete solution. My attempt to expand
> it in TS-1031 did not end well, due to the lack of locks from NetHandler.
> At that point I was looking at a significant change, so I decided that I
> should try to do the best thing, rather than a minimalist hack which would
> not really be that much smaller a change.
>
> Friday, December 9, 2011, 11:38:47 AM, you wrote:
>
> > Could you be more specific about the problem (e.g. big numbers)?
> >  VConnection locking works fine in all cases that I know of.  There are
> > some issues with closing and deallocation with threading and the higher
> > layers, but these could be solved in a number of ways.  Smart pointers is
> > one (and probably not a bad one) although it isn't a panacea, just a
> > mechanism.  Similarly, a circular buffer queue is just a different way of
> > viewing the data structure.  Sure, it has some advantages, particularly
> in
> > exposure to deallocation and locking, but it isn't a silver bullet
> either.
> >  I'd like to understand the core problem.
>
>

Re: TS-857, TS-934, TS-1031

["Alan M. Carroll" <am...@network-geographics.com>]

The core problem is that VC connection locking does not work. VCs are deallocated while still in use which leads to crashes. In fact, the general handling of VCs in NetHandler::mainNetEvent doesn't lock the VCs at all, so the cross thread access can't be safe. Moreover, because all the VC references are stored as raw pointers, there's no good way to safely access them across threads or even detect deallocation. I tried putting in generation numbers but that was insufficient (you can still get crashes because the virtual function pointer has been swizzled by the deallocator before you even get to the VC). All the crashes in the listed bugs stem from VC locking failures leading to cross thread corruption.

I've been at quite a loss on how to fix this.

The two root causes, IMHO, are

1) VCs are ephemeral and stored purely by pointer, which means the accessor cannot have thread safe access. You can use pointers if your objects don't evaporate asynchronously. The use of raw pointers also means there's no way to detect dangling references.

2) Closing and deallocation are conflated, so that when a VC needs to be closed, it is also de-allocated. There are good reasons to get the close done ASAP but the de-allocation is not nearly a time critical.

I am looking at using smart pointers to alleviate these two issues. However, that creates problems because the current queue mechanisms require the use of raw pointers. AFAICT the point of these is to minimize allocation on the fast path. The circular buffer is an attempt to have as little allocation impact as possible, otherwise I would just use a standard allocate per element style.

Experimentally, the fix I did for TS-934 had a strong effect on reducing the problem, although it was not a complete solution. My attempt to expand it in TS-1031 did not end well, due to the lack of locks from NetHandler. At that point I was looking at a significant change, so I decided that I should try to do the best thing, rather than a minimalist hack which would not really be that much smaller a change.

Friday, December 9, 2011, 11:38:47 AM, you wrote:

> Could you be more specific about the problem (e.g. big numbers)?
>  VConnection locking works fine in all cases that I know of.  There are
> some issues with closing and deallocation with threading and the higher
> layers, but these could be solved in a number of ways.  Smart pointers is
> one (and probably not a bad one) although it isn't a panacea, just a
> mechanism.  Similarly, a circular buffer queue is just a different way of
> viewing the data structure.  Sure, it has some advantages, particularly in
> exposure to deallocation and locking, but it isn't a silver bullet either.
>  I'd like to understand the core problem.

Re: TS-857, TS-934, TS-1031

[John Plevyak <jp...@acm.org>]

Could you be more specific about the problem (e.g. big numbers)?
 VConnection locking works fine in all cases that I know of.  There are
some issues with closing and deallocation with threading and the higher
layers, but these could be solved in a number of ways.  Smart pointers is
one (and probably not a bad one) although it isn't a panacea, just a
mechanism.  Similarly, a circular buffer queue is just a different way of
viewing the data structure.  Sure, it has some advantages, particularly in
exposure to deallocation and locking, but it isn't a silver bullet either.
 I'd like to understand the core problem.

john

On Tue, Dec 6, 2011 at 12:22 PM, Alan M. Carroll <
amc@network-geographics.com> wrote:

> The problem with all of these is that locking for the VConnections is done
> poorly, if at all, and closing a VConnection and de-allocating are
> conflated. Changing that is hard, though.
>
> My current plan is related set of changes
>
> 1) Use a smart pointer (based on lib/ts/Ptr.h) instead of raw pointers,
> separating out do_io_close and free. The former can be done as it is now,
> but the latter only when the last pointer is dropped.
>
> 2) Write a new queue class that uses lib/ts/Vec.h to create an expanding
> circular buffer.
>
> 3) Use (2) to replace the current queue support for read_ready_list and
> write_ready_list. The problem this solves is that these currently use
> intrusive list pointers and that just won't work with (1). OTOH the
> instrusive lists do avoid allocations for elements in the list. The
> circular queue of (2) should give us the same level of speed with an
> acceptable amount of allocation (should be roughly constant per process, as
> eventually the queue will get big enough).
>
> 4) When pulling an element off the ready lists, lock it. If it can't be
> locked, put it on the back of the queue. This should not have a big impact
> because AFAICT the only reason the lock will fail is because it's being
> closed in another thread in which case rapid processing isn't important.
>
>