Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout

On 18/01/2013 6:47 p.m., Alex Rousskov wrote:
> On 01/17/2013 10:28 PM, Amos Jeffries wrote:
>> On 18/01/2013 5:37 p.m., Alex Rousskov wrote:
>>> On 01/17/2013 06:35 PM, Amos Jeffries wrote:
>>>>> Amos, do you remember what "partially open FD" in the following comment
>>>>> meant, exactly? Why cannot we just call comm_close() to cleanup?
>>>> It means we have issued socket() and connect() OS calls. fd_table[]
>>>> state probably exists. But we are still waiting on either TCP response,
>>>> or the In-Progress event to run and the FD to be confirmed as open.
>>>> This is signalled by (temporaryFd_ >= 0).
>>> Why cannot we just call comm_close() to cleanup Comm state?
>> Because comm_openex() and comm_close() are not "symmetrical" in what
>> they do. comm_close() involves a lot of handlers cleanup and close code
>> for state objects which simply do not exist yet.
> Why non-existing handlers and objects are a problem?
>
> We have a write handler and a close handler registered with Comm, right?
> Why not call comm_close() to clean them up (from Comm point of view) and
> close the descriptor? We do not need those handlers to be actually
> called (we may even cancel them before calling comm_close). We just need
> to clear Comm state and close the descriptor.
>
> Will comm_close() break if we call it instead of reaching into Comm guts
> and manipulating its tables? If yes, what is Comm missing? Perhaps we
> can give that missing piece to Comm so that we can comm_close()?

* Notice that until ConnOpener has finished the FD is not open, as in
"if(isOpen(fd)) return;" produces false, so comm_close() will exit
before it gets to any cleanup. Removing that check will result in
trouble processing some FD types which currently use comm_close().
* The registered close handler is earlyAbort which flags the FD as error
instead of timeout or success.
* fd_close(temporaryFd_), which we use already, cleans up all the
non-ConnOpener state which is associated with the FD.

It is simpler just to prune out the two bits of ConnOpener state in
ConnOpener itself rather than rely on the generic close sequence, which
has to do a lot of checking for irrelevant things.
The whole fiddling with write handler complexity is a hack internal to
ConOpener anyway. Otherwise it could just be SetSelect(..., NULL,...)
and be done.

>
>> The FD has only had socket() and connect() done on it and a few specific
>> things done to the fd_table. One of which is registering the
>> calls_.earlyAbort_ handler as what gets run on comm_close() in case the
>> Squid shutdown/restart sequence or TCP close happens during this Jobs
>> operations. If we use comm_close() most of the required cleanup will
>> never happen.
> Please note that I am _not_ suggesting to call comm_close to clean up
> our job state (like a lot of Squid code does) even though I believe it
> would be possible if doneAll() is adjusted accordingly.
>
> I am just trying to find a way to call comm_close() to clean up Comm
> state without violating Comm APIs. We will clean the job's state in
> addition to calling comm_close().

ConnOpener lives in the limbo area between fd_open()/fd_close() which
sets up the TCP state on an FD and comm_open()/comm_close() which setup
and tear down the mountain of comm transaction I/O state on an FD.
  From any point after ConnOpener move temporaryFd_ into conn->fd
(finished with success) comm_close() is the suitable way to cleanup. On
the other error/timeout exit points from ConnOpener fd_close()+unwinding
is the more appropriate way to cleanup [because Comm failed to setup the
FD] and nothing outside of ConnOpener is ever informed about an FD#
which enforces that condition.

You may indeed be able to re-write comm_close() such that it will work
on any FD, including one which failed to reach success in ConnOpener.
But I think that is a much more difficult/larger project outside the
scope of the priority bug-fixes Rainer is working on here.

Amos
Received on Fri Jan 18 2013 - 11:54:26 MST