[squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"

[Alex Rousskov]

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;
>     }
> }


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?

Why are we prioritizing completeDataExchange() over
writeForwardedReply(delayedReply)?

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.



> Probably the completeDataExchange should be renamed to
> completeDataDownload. It is used only while we are downloading objects
> (not uploading).

Agreed.



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