Re: [PATCH] helper closing sequence cleanup

On 05/05/2009 06:38 AM, Amos Jeffries wrote:
> This one will definitely need a review and second opinion.
>
> While looking over the helper shutdown sequence to investigate bug 2648.
>
> If I understand it now the two states for end-of-life are:
> - shutdown :: closure is needed, but for some reason it can't be done
> immediately.
> - closing :: actual comm_close() has been called on the read socket.

Could you please document the corresponding helper_server flags,
n_running, and n_active? And the stats members as well? This will help
those reviewers who do not know the original code intent behind all
those state variables and flags :-).

The patch overall seems to be cleaning up the code quite a bit, thank you.

Should we make _helper_stateful a kid of _helper to avoid duplicate
declarations and reduce the number of things we need to document? This
may also allow you to write one DoAbort method for the two helper kinds.

I am concerned about n_active management in the current code because it
is decremented in helperShutdown even when we call "continue" and do
almost nothing for that particular "link". Your patch seems more logical
than the current code, but still it decrements the n_active counter
before something specific has happened. Perhaps n_active documentation
would clarify that, but maybe we should move n_active change to
helperDoAbort or elsewhere?

Please add a description to helperDoAbort and friends. Can this be
renamed to match shutdown or closing state terminology (e.g.,
helperDoShutdown) or are we doing something different than shutting down
or closing here? I think you have a good beginning of this documentation
in this email already.

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?

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?

Thank you,

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 Tue May 05 2009 - 16:24:54 MDT