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

Alex Rousskov wrote:
> 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.

I've checked the RFC 959, I had it slightly wrong. ABOR is for data
unexpected closure, not control unexpected closure.

(NP: what you have committed will work, so ignore the following if you
want to save time).

So...

For expected closures of data fd, the close handler needs to be removed
before closure.

If the server closes data fd from that end. I *think*, without checking,
that is expected closure the existing code already handles.

For unexpected closures of data fd (from squid end), we SHOULD send
"ABOR" down the _control_ fd if that is still open. Leaving the control
fd otherwise alone to keep state running. (Or since squid has no use for
an unused open ctrl.fd, leaving failed() to close it properly.)

ref RFC 959 section 3.2 pg 18 -
  The server MUST close the data connection under the following conditions:
          1. The server has completed sending data in a transfer mode
             that requires a close to indicate EOF.
          2. The server receives an ABORT command from the user.
          4. The control connection is closed legally or otherwise.
(non-relevant conditions elided)

>
> 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).

What you committed is valid, just not a nice transfer closure.

There is no failover or recovery on the current design. I'll make a doc
comment about what should be done there when its eventually cleaned up...

<snip>
>
> I am not sure I follow.

I was just waffling and confusing myself. Sorry. Not relevant at this stage.

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

You have good instincts, the behavior there has possibilities of failure
recovery which are currently missing. Worth looking at One Day (tm)

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

Yes.
Just getting the state of the new handler to fit and work right for now.

Amos

-- 
Please use Squid 2.7.STABLE4 or 3.0.STABLE9