Re: [RFC] shutdown stages

On 08/09/2011 09:37 PM, Amos Jeffries wrote:
> On Tue, 09 Aug 2011 10:13:55 -0600, Alex Rousskov wrote:
>> On 08/08/2011 08:15 PM, Amos Jeffries wrote:
>>
>>> Okay we seem to agree on all the prerequisites and problems. Just the
>>> algorithm of choice differs.
>>
>> Yes, that is rather likely.
>>
>>> So separating the central issues into changes. Ordered by severity.
>>>
>>>
>>> 1) pending async queue after the grace period is over / shutdown
>>> functions scheduling calls
>>>
>>> Insert a checking member to SignalEngine as an intermediary between
>>> other code and StopEventLoop().
>>>
>>> Problem: Can we get to Async queue size() from SignalEngine members?
>>
>> Can we just implement the following?
>>
>> If in shutdown and there are no events, quit the main loop.
>> Otherwise, keep iterating the main loop.
>>
>> where "event" includes all scheduled async calls and scheduled timed
>> events, ignoring the ones after the shutdown timeout.
>>
>> In pseudo code, this can be written as
>>
>> main()
>> {
>> initial setup
>> while (!in_shutdown || haveWorkToDo())
>> do a single main loop iteration
>> return 0;
>> }
>>
>> onShutdown() {
>> in_shutdown = true;
>> schedule forceful termination of master transactions in N seconds
>> stop accepting new master transactions
>> other pre-shutdown steps...
>> }
>>
>> onMasterTransactionEnd() {
>> ...
>> if (in_shutdown && there are no more master transactions)
>> schedule and async call to perform shutdown steps
>> }
>>
>> haveWorkToDo() will have access to the number of scheduled async calls
>> and [relevant] timed events. That should not be hard to write.
>>
>> The key here is that almost everything proceeds as usual while we let
>> the existing master transactions end. The main loop runs until the
>> very end.
>>
>
> What about the read/write handlers waiting on a FD signal? they have no
> addEvent() entries, no ScheduelCall() entries and are a common state of
> operation.

Any I/O handler should belong to some higher-level object such as an
http_port listening job or a master transaction. It should be the
responsibility of those higher-level objects to cancel their I/O to
avoid wasteful I/O (Comm can handle I/O for gone objects, IIRC, but it
is a waste).

> We would need to add a call to prune the idle pconn connections on both
> sides. That still leaves a hopefully small, but existing group of FD.

Yes, the idle connection pools will need to be notified in "other
pre-shutdown steps..." above. There should probably be a simple
dedicated API for other modules to register for such "shutdown started"
notifications.

> The point of the ticker was to check for these being present and
> allowing 1 tick more for their signal to happen.

I do not understand the "their signal to happen" part, but the above
scheme avoids artificial delays and magic constants like a "one 1-second
tick". We just move as fast as we can instead.

>>> 2) useless waiting / grace period
>>>
>>> How about a shutdown "ticker" style event?
>>> Rather than a blanket 30-second grace period jump. We schedule a
>>> shutdownTick() every second starting with shutdown signal arrival. The
>>> tick event checks to see if a) all transactions are over, or b) grace
>>> period has expired since its first call. Either way things can be kicked
>>> along, else it re-schedules itself in one more second.
>>>
>>> The alternative of scheduling it as a call could lead to very tight
>>> loops and CPU consumption. So I'm avoiding it for now despite
>>> potentially being a better solution in the long run. Events can be
>>> easily converted in a second round of optimization.
>>
>> With the design sketched above, you do not need a ticker. If
>> transactions stop earlier, we quit the main loop earlier. If
>> transactions keep running, the shutdown timer will expire and its
>> handler will forcefully kill all running transactions (while the main
>> loop still runs!).
>>
>>
>>> Problem: How to best detect the number of active transactions?
>>
>> We need a master transaction class that will provide a forceful
>> termination API. There will be a list of master transaction objects. The
>> size of the list will determine the number of master transactions. The
>> list and the API will be used to kill master transactions that take too
>> long.
>
> Okay. Nice. I agree we need to end up with this. But right now it
> requires a lot of additions just to work. I'm looking for small steps
> towards that design, which will show benefits in the short term as well.
>
> Thus the ticker, which can happen now and change from
> eventAdd(haveWorkToDo()) to while(haveWorkToDo()) when the master
> transaction management is ready.

It is difficult for me to judge since I am not doing the coding, but it
feels to me that the amount of required/essential/difficult changes is
about the same in both approaches. Thus, I would recommend doing this
right from the start rather than adding more complex hacks that would
need to be dealt with later. I do consider multi-stage shutdown complex,
no mater what the implementation is. It is your call though.

>>> Do we have any global clients + internal transactions count already
>>> which I've been too thick to see?
>>> If not...every transaction manager object seems to have an
>>> AccessLogEntry in its members. (Recall that is why I suggested that
>>> object became Xaction master slab). A static counter of transactions in
>>> there could be used.
>>
>> We need more than a counter (see above). Ideally, we need to split
>> HttpRequest into MasterTransaction and pure HttpRequest. For now, we
>> could just add a MasterTransaction member to the HttpRequest. We could
>> reuse and adjust the existing AccessLogEntry for that, but I would
>> rather not mix the two completely different uses to save a few bytes!
>>
>
> It already is split more than enough ...
>
>
> * ConnStateData manages on-wire details about the transaction.
> - handles I/O for TLS coding (SSL and/or chunking)
> - performs parse and pipelining to split individual transactions out
> from an FD data stream.
> - abused to do the 1xx handling pieces of the transaction on its own
> volition.
>
> ** HttpParser performs the parsing operations up until the last byte of
> headers is actually received.
> - abused to be run along with parsing C-functions by ConnStateData
> (I mention this because its part of the transaction which exists
> outside the transaction states)
>
> * ClientSocketContext is as close to a master transaction functionality
> as exists in one place.
> - performs the simple duties of a state enumeration and changes to
> data in a ClientHttpRequest.
> ** except doCallouts() is a ClientHttpRequest method which runs these
> context states from outside the context itself
> ** except that adaptation reply functions are done by
> ClientHttpRequest directly
>
> ** ClientHttpRequest is as close to a master transaction data-only slab
> as exists.
> - only handles request pieces, no 1xx or reply data goes near it
> (except adaptation replies)
> - abused to manage adaptation state changes in doCallouts(), external
> to the ClientSocketContext state machine
> - (abused?) to do storage range packing states on its own
> ** data is not actually parsed into it, but into objects elsewhere
> (HttpParse, HttpRequest) then copied back irregularly by third-party
> code later on
>
> *** HttpRequest the headers mime parser and resulting data cache/manager
> - abused to be a miscellaneous data cache when probably
> ClientHttpRequest should have been used.
>
> *** AccessLogEntry miscellaneous data cache.
> - abused? to store data when its owner class has duplicate copies of
> all data anyway
> - only filled at the end of the transaction after much data has
> already been lost due to the source Job closing.
>
>
> There is a little bit of scope cleanup needed before transaction master
> concept can be applied properly there. This is way outside the
> "optimizing shutdown" scope now.
>
>
> Now that I think about it. That above is better coverage of the overview
> than what is documented so far. Please check and criticise the details,
> I'll add this to the docs soon.

Sorry, this is too vague for constructive criticism. I do not want to
waste your time on arguing about poorly defined concepts. I am sure most
of the above is or can be correct, depending on one's point of view.

On the design level, I can only note that master transaction should be
aware(**) of all associated activities such as virgin request, 1xx
control messages, virgin response(s), and adaptations. Currently,
HttpRequest is as close as we can get to that. The other parts you
describe are too specialized and restricted in their scope to play that
role, IMO.

The only serious problem with HttpRequest is that there may be several
such objects per master transaction. I would not be surprised if some
code even creates and stores HttpRequest objects unassociated with any
specific master transaction. That would be difficult to fix, but not
impossible.

HTH,

Alex.
(**) This awareness does not imply detail knowledge. It could be limited
to having a link to the corresponding job in many cases. At the minimum,
a master transaction should be able to kill all related jobs.
Received on Wed Aug 10 2011 - 14:43:41 MDT