FtpGateway: Unexpected ABORTED suffixes in %Ss #267
Conversation
While downloading/uploading files using HTTP GET/PUT requests with ftp URI scheme, Squid completes these transactions successfully but sometimes logs the ABORTED suffix, e.g., TCP_MISS_ABORTED. This happens because Squid postpones completing the corresponding StoreEntry until getting 221 reply from FTP server, even though the server has sent all response bytes already. So if the client closes the client-to-Squid connection before that, the cleanup code in ConnStateData::terminateAll() finishes the transaction which is logged as 'ABORTED' because the StoreEntry is still in the STORE_PENDING state. Now we complete the entry, calling Ftp::Gateway::completeForwarding() as soon as Squid got all FTP response bytes.
src/clients/FtpGateway.cc
Outdated
| @@ -1008,6 +1008,11 @@ Ftp::Gateway::processReplyBody() | |||
|
|
|||
| entry->flush(); | |||
|
|
|||
| if (theSize >= 0 && data.payloadSeen >= theSize) { | |||
| markParsedVirginReplyAsWhole("whole virgin body"); | |||
| completeForwarding(); | |||
There was a problem hiding this comment.
This fixes 'ABORTED' for ftp 'GET' transactions. We cannot add this to ftpReadTransferDone() similarly as to ftpWriteTransferDone() for 'PUT' transactions because for 'GET' transactions 226/250 response may come after Squid sends the whole downloaded file to the client (and client may have closed the connection before 226/250). Note that this does not happen for 'PUT' transactions, because ftpWriteTransferDone() initiates the response itself, i.e., the client receives the response only after Squid gets 226/250.
Another method (serverDataComplete) is called instead to take care about doneWithAdaptation() condition. Also serverDataComplete() for all other ftpSendReply() cases, such as ftpReadMkdir().
Partially undone 74cf59a (serverDataComplete() method). markStoredReplyAsWhole() is a common place for marking the reply as complete for both adaptation and non-adaptation cases. Special care should be taken about not calling complete() twice.
Before this fix, the (fully completed) entry was completeTruncated() instead, 'WITH_CLIENT' (a kind of error indicator) was logged in err_detail. Also undone branch adjustments in FwdState::completed(): there can be scenarios when the entry status != STORE_OK (e.g., due to reforward() depending on many factors).
| @@ -1257,6 +1263,7 @@ Ftp::Gateway::loginFailed() | |||
|
|
|||
| // add it to the store entry for response.... | |||
| entry->replaceHttpReply(newrep); | |||
| fwd->markStoredReplyAsWhole("loginFailed"); | |||
There was a problem hiding this comment.
This fixes the 'entry->completeTruncated()' problem and adding 'WITH_CLIENT' to err_detail. I fixed other cases in FtpGateway as well.
| if (e->store_status == STORE_OK) { | ||
| debugs(17, 3, "No, the entry is STORE_OK already"); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
I checked that FwdState::completed() would hit the 'e->store_status == STORE_OK' assertion (added in e322b47) and removed that assertion. In that test (where server responds with an HTTP 502 reply) I adjusted the code so that reforward() would return 'true' when called the first time (so that markStoredReplyAsWhole() does not complete the entry). Also note that this scenario should never happen in FTP because reforward() always returns false (FTP code never sets ENTRY_FWD_HDR_WAIT).
as I am going to post it as a separate PR.
While downloading/uploading files using HTTP GET/PUT requests with ftp
URI scheme, Squid completed these transactions successfully but
sometimes logged the ABORTED suffix, e.g., TCP_MISS_ABORTED. This
happened because Squid postponed completing the corresponding StoreEntry
until getting 221 reply from FTP server, even though the server has sent
all response bytes already. So if the client chanced to close the
client-to-Squid connection before that, the cleanup code in
ConnStateData::terminateAll() would finish the transaction which logged
as 'ABORTED' because the StoreEntry was still in the STORE_PENDING
state.
Now we complete the entry as soon as all FTP response bytes have been
received. Similar cases (where Ftp::Gateway generates the final response
itself) were fixed as well.
Also added some missing markStoredReplyAsWhole() calls where FTP code
generates replies. Without this, FwdState cleanup code called
completeTruncated() for the entry and %err_detail logged 'WITH_CLIENT'.