[PATCH] helper closing sequence cleanup

From: Amos Jeffries <squid3_at_treenet.co.nz>
Date: Wed, 06 May 2009 00:38:09 +1200

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.

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 - 12:38:17 MDT

This archive was generated by hypermail 2.2.0 : Tue May 05 2009 - 12:00:02 MDT