Alex Rousskov wrote:
On Wed, 2008-09-24 at 13:37 +1200, Amos Jeffries wrote:
------------------------------------------------------------
revno: 9220
committer: Alex Rousskov <[EMAIL PROTECTED]>
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
