Re: [PATCH] helper closing sequence cleanup

Alex Rousskov wrote:
> On 05/05/2009 04:49 PM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> Can we remove flags.closing and rely on the corresponding wfd state
>>> instead? Is wfd always set to -1 when we set flags.closing to 1?
>> Did I not comment that some closures are rfd closed first, others are
>> wfd closed? The helepr closing happens both ways depending on whether
>> its Abort or Graceful close happening.
>
> I am sorry for not being able to connect the dots faster. I will reshape
> the question: Can we remove flags.closing and rely on wfd and rfd state
> instead? If one of the descriptors is negative, can we assume that we
> are in the flags.closing state?

Ah. click. Yes I believe we should be able to. Will have to go through
and check before doing it
.
IMO the final test may need to be (wfd>0 && rfd>0 && shutdown==0).

>
>>> What happens to the helper state after we set flags.closing and
>>> comm_close closes the descriptor? We do not have a flags.closed state.
>>> Is that because the helper is completely destroyed when comm_close does
>>> its job and calls us back?
>> As far as I can tell this ....
>>
>> +/**
>> + * Abort reading from this helper immediately.
>> + *
>> + * This is a sharp short closure for the helper which may be used
>> + * for quick closure or error aborting.
>> + * Any pending requests are abandoned.
>> + *
>> + * It's used during helprShutdown, but that is just graceful enough
>> + * to schedule the close and wait for existing queues to drain.
>
> The above two paragraphs contradict each other, IMHO. A "sharp short
> closure with pending requests abandoned" cannot be "used during" a
> "graceful" shutdown that "waits for existing queues to drain". What I am
> missing here?

The shutdown splits all helper state objects into two groups.
  Group 1 are currently inactive and it does the Abort on them hoping
they don't actually have a queue to be handled (maybe that is a
guaranteed state of being inactive?).
  Group 2 are busy on some fashion (all those continue lines) and it
sets the state to perform Graceful async shutdown on those at some later
time. All gracefulness is outside the Abort call actions themselves.

I think the shutdown may benefit from doing the Graceful action always
and waiting. But that will need testing under load to see whether such
async shutdown is faster or slower than the current Abort with many helpers.

I've made the shutdown calls return the number of remaining 'active'
helpers so that squid shutdown can in future use that result to sensibly
handle the case of pending helpers which are currently just adding to
the breakage of the comm shutdown.

>
>> + */
>> +static void helperDoAbort(helper_server * srv);
>> +static void helperStatefulDoAbort(helper_stateful_server * srv);
>> +
>> +/**
>> + * Graceful closure of the helper.
>> + * Write socket is closed and helper marked as closing so that
>> + * further requests are not queued.
>> + *
>> + * Pending queued items are currently abandoned (need to fix that),
>> + * but requests already sent to the helper are waited for
>> + * and the sockets only fully closed when none remain to receive back.
>> + */
>> +static void helperDoGracefulClose(helper_server * srv);
>> +static void helperStatefulDoGracefulClose(helper_stateful_server * srv);
>>
>> Then the close handler associated with the rfd is *Free.
>>
>> This is why I named them that way around. Because marking thw wfd and
>> closing flags and waiting for the state to fall naturally into unused
>> and do its free looks more graceful than closing read and abandoning data.
>>
>> I'd really like these to always be done the same way in a simple Close()
>> call, but can't see a good way to do it easily yet.
>
> Can I interpret the above that flags.closing means "we have called
> comm_close on at least one of the descriptors"?

Well, all I know right now is that the guarantee is that at the point
comm_close() gets called that it must be in state flags.closing==1.

Whether the flags.closing can be set without com_close I don't now yet.
But I doubt it.

>
> Thanks,
>
> Alex.
>
>
>>>> I found a few things not quite right with the handling assuming both of
>>>> the above are true ...
>>>>
>>>> * line 1553 :: all of the comm_close calls appeared to be matched with
>>>> flags.closing=1 except this one in the stateful delayed shutdown
>>>> closure. This is probably the actual cause of bug 2648.
>>>>
>>>>
>>>> * line 1381, 1424 :: The lookups for next available helper were
>>>> skipping helpers with shutdown pending. But identifying those currently
>>>> closing as candidates. This leaves the helpers in an error-state closure
>>>> as candidates for use. It may not be an issue now with immediate comm
>>>> callbacks, but considering the closing state has a potential async step
>>>> in the middle I don't think its a good idea to ignore any longer.
>>>>
>>>>
>>>> * helper*Shutdown() :: were marking helpers as non-active before
>>>> checking to see if they had pending work to do, instead of marking only
>>>> the closed helpers as inactive. The effects of this are probably hidden
>>>> behind the comm shutdown closures on reconfigure.
>>>>
>>>>
>>>> * line 1011, 1123 :: error-state closures are called directly on the
>>>> FD, but the helper state is not updated to indicate the closure state of
>>>> the helper state object. At the very least flags.closing=1 needs to be
>>>> done here too. I've opted for making these closures part of the
>>>> hard-close (see below)
>>>>
>>>> The rest of the patch is removing duplicate code around comm_close()
>>>> into the two states of closure ( x2 for the two types of helper):
>>>>
>>>>
>>>> helper*DoAbortClose()
>>>>
>>>> Where the read socket for receiving further data goes down first. The
>>>> helper and its pending requests are abandoned. The shutdown terminator
>>>> is pushed to the write socket. The callbacks cleanup the remaining state
>>>> and close the write.
>>>>
>>>> I've pulled that closure code out of the shutdown sequences, this moves
>>>> a step closer to merging the two. Also allows the error-case closure
>>>> noted above to use the full close sequence on the state object with
>>>> windows shutdown etc. Which was missing previously.
>>>>
>>>> This one has windows shutdown handlers.
>>>>
>>>>
>>>> helper*DoGracefulClose()
>>>>
>>>> Where the write socket is closed so no more data can be pushed at the
>>>> helper, but the read socket remains open for the last pending data
>>>> results to come back.
>>>>
>>>> This is used for delayed shutdown completion, and for closure of the
>>>> write socket during read socket shutdown. Also enforces mark of the
>>>> closing flag for when its the first point of closure.
>>>>
>>>> This one I'm not sure of why, but its missing windows shutdown
>>>> handlers. The unwrapped comm_close may screw with the HANDLE closure.
>>>> May need to become a NOP for windows.
>>>>
>>>>
>>>> Amos
>>>>
>>
>

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE14
   Current Beta Squid 3.1.0.7