Re: /bzr/squid3/trunk/ r9220: Added Comm close handler for the data channel of FtpStateData

On Wed, 2008-09-24 at 13:37 +1200, Amos Jeffries wrote:
> > ------------------------------------------------------------
> > revno: 9220
> > committer: Alex Rousskov <rousskov_at_measurement-factory.com>
> > branch nick: trunk
> > timestamp: Tue 2008-09-23 08:49:50 -0600
> > message:
> > Added Comm close handler for the data channel of FtpStateData
> > transaction in preparation for officially dropping connect callbacks for
> > closing descriptors.
> >
> > The data channel can be opened and closed a few times and the descriptor
> > must be kept in sync with the close handler. I factored out the
> > open/closing code into a simple FtpChannel class. That class is now used
> > for both FTP control and data channels.
> >
> > The changes resolve one XXX discussion regarding FTP not having a close
> > handler for the data channel. On the other hand, adding a second close
> > handler attached to the same transaction is not a trivial change as the
> > side-effects of Squid cleanup code are often illusive.
> >
> > For example, I suspect that FTP cleanup code does not close or even
> > check the control channel. I added a DBG_IMPORTANT statement to test
> > whether the control channel remains open. Or should that be an assert()?
> >
> > I think that only one out of the two callbacks can be dialed because the
> > close handler executed first will invalidate the transaction object.
>
> FTP data channel can open close any time.

Agreed.

> It's close handler needs to only
> handle the fd, with no implications on the other FTP state.

"Yes" if the close handler purpose is to handle planned closing of the
data channel. "Not so sure" if the handler purpose is to handle
unexpected data channel closures only.

Before the above change, there were no close handler for the data
channel at all, so we can say that the old code did not want to cleanup
during planned data channel closing or that the cleanup code was called
before comm_close.

Currently, the close handler for the data channel should only deal with
unexpected closures. When the closure is unexpected, the current code
will abort the entire FTP transaction. For planned closures, we remove
the handler before closing so it is not involved, just like before.

It is possible that a different overall design would be better, but I
tried not to disturb the exiting code when addressing a specific issue
(lack of a closing handler for comm connect).

> The FTP general close handler should close the control channel with a
> "QUIT\0" or "ABOR\0" (per RFC), and then close the data channel
> (discarding anything in-transit on ABOR, reading to end of current in
> buffer and closing on QUIT).

I am not sure I follow. The close handler is called when the descriptor
is closing so the handler cannot send anything on that channel. This is
a low-level comm close handler, not some high-level "quit nicely"
routine.

> So the shutdown procedure for FTP should be calling the control channel
> close handler and letting it handle the data channel cleanup.

Yes, probably, but we may be talking about different "handlers" here. My
commit message talks about "close handlers" and "FTP cleanup code".
Those are related but not the same. Transaction cleanup code is called
when transaction ends or is aborted. The close handlers (if any) are
called when a channel descriptor is closed.

My commit comment mentions that the cleanup code does not seem to check
whether the control channel is closed, which seems odd to me, but I
could easily overlook something. Normally, the cleanup code should close
all still-open descriptors owned by the transaction.

FWIW, I tried not to change the normal shutdown procedure in FTP. If
correct, my changes should affect unexpected data channel closures only.

> However,
> this would not play nicely on shutdown right now. Just on regular
> connection closes.
>
> FTP still needs a re-work cleanup at some later date which can do this
> sequence checking and fixup. Also to get rid of many global functions and
> do translations of generated pages properly from templates.

I agree that we need to clean it up. IMO, it is not as bad as the client
side though, so we may want to concentrate on that first if we cannot do
it in parallel.

Thank you,

Alex.
Received on Wed Sep 24 2008 - 05:08:11 MDT