Re: [PATCH] helper closing sequence cleanup

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?

>> 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?

> + */
> +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"?

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
>>>
>>
>
>
Received on Wed May 06 2009 - 02:44:46 MDT