On 03/14/2016 06:33 PM, Alex Rousskov wrote:
On 03/13/2016 01:57 PM, Christos Tsantilas wrote:
On 03/10/2016 11:35 PM, Alex Rousskov wrote:
On 03/10/2016 12:14 PM, Christos Tsantilas wrote:
if (master->serverState == fssHandleDataRequest) {
+ if (!master->userDataDone) {
...
+ originDataDownloadAbortedOnError = (originStatus > 400);
+ return;
+ }
+
+ completeDataExchange();
+ } else {
+ if (delayedReply != NULL) {
+ writeForwardedReply(delayedReply.getRaw());
+ delayedReply = NULL;
+ }
+ }
The above logic looks correct to me, but I feel like I am reading an
inside-out code. Please consider this instead:
I did not follow your suggestion here.
The completeDataExchange should be called only if we are downloading
data.
I think your code is [still] correct but, thanks to your comments, I now
understand where [my] confusion about its meaning was coming from. I
suggest the following (functionally-equivalent) code:
void
Ftp::Server::stopWaitingForOrigin(int originStatus)
{
Must(waitingForOrigin);
waitingForOrigin = false;
// completeDataExchange() could be waitingForOrigin in fssHandleDataRequest
if (master->serverState == fssHandleDataRequest) {
// Depending on which side has finished downloading first, either trust
// master->userDataDone status or set originDataDownloadAbortedOnError:
if (master->userDataDone) {
// We finished downloading before Ftp::Client. Most likely, the
// adaptation shortened the origin response or we hit an error.
// Our status (stored in master->userDataDone) is more informative.
// Use master->userDataDone; avoid
originDataDownloadAbortedOnError.
completeDataExchange();
} else {
debugs(33, 5, "too early to write the response");
// Ftp::Client naturally finished downloading before us. Set
// originDataDownloadAbortedOnError to overwrite future
// master->userDataDone and relay Ftp::Client error, if there was
// any, to the user.
originDataDownloadAbortedOnError = (originStatus >= 400);
}
return;
}
if (delayedReply) {
writeForwardedReply(delayedReply.getRaw());
delayedReply = nullptr;
}
}
It is OK, fixed locally here.
The only remaining doubt in my mind is the combination of delayedReply
and fssHandleDataRequest state. The above code appears to assume that,
in fssHandleDataRequest, delayedReply is either always nil or never
important. Is that really true? delayedReply is set in
Ftp::Server::writeForwardedReply() which is called from lots of places,
including Ftp::Server::handleDataReply(). May it be set in
fssHandleDataRequest?
Good question.
The writeForwardedReply inside Ftp::Server::handleDataReply is called
only if the transfer of the file on server side did not started ,
because of an error. In this case the server reply forwarded as is.
In this case the waitingForOrigin is not clear that it will be always
false or not, and it is possible that the delayedReply used here too.
(now the watingForOrigin is false because of the order the asyncCals are
called)
The delayedReply can not be ignored because at least must be set to null.
Moreover In this case it is not bad idea to forward the server reply as
is (eg "the file does not exist on server" error).
I have to admit that your proposal to forward the delayedReply if it is
set, else use completeDataExhange is the correct solution for this.
But we must check if we are in fssHandleDataRequest state before call
the completeDataExchange.
Why are we prioritizing completeDataExchange() over
writeForwardedReply(delayedReply)?
You are right. We must not do it.
And if we are doing the right thing, should we either assert that with
Must(!delayedReply) in the beginning of the fssHandleDataRequest clause
or clear the delayedReply, if any, in that clause?
+ debugs(33, 5, "Transfering from FTP server is not complete");
+ debugs(33, 5, "Transfering from FTP server terminated with an error, adjust
status code");
s/FTP server/FTP origin server/ to minimize the chance of this "FTP
server" phrase being misinterpreted as Ftp::Server.
OK on this
Probably the completeDataExchange should be renamed to
completeDataDownload. It is used only while we are downloading objects
(not uploading).
Agreed.
Should I do it in this patch commit?
All of these are pretty much cosmetic changes that do not alter the
proposed patch functionality AFAICT. IMO, if there are no objections,
the polished patch should go into trunk (see above regarding v3.5).
I am attaching a new patch here, if no objection I will apply this to
trunk.
Please do, especially if my concerns regarding the combination of
delayedReply and fssHandleDataRequest state are easily addressed.
Thank you,
Alex.
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
