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 <[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... 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
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 <[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. 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.
Re: /bzr/squid3/trunk/ r9220: Added Comm close handler for the data channel of FtpStateData
> > 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. It's close handler needs to only handle the fd, with no implications on the other FTP state. 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). So the shutdown procedure for FTP should be calling the control channel close handler and letting it handle the data channel cleanup. 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. Amos
