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

[Christos Tsantilas]

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