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

[Christos Tsantilas]

On 03/14/2016 06:17 AM, Amos Jeffries wrote:
> On 14/03/2016 8:57 a.m., Christos Tsantilas wrote:
>> Hi all,
>>
>> I made all of the fixes requested by Alex.
>> Please see below for my comments.
>>
>> 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:
>>>
>>> // HttpReply in fssHandleDataRequest lacks status code
>>> if (master->serverState == fssHandleDataRequest)
>>>       originDataDownloadAbortedOnError = (originStatus > 400);
>>>
>>> if (!master->userDataDone) {
>>>       ... your big comment here ...
>>>       // HttpReply in fssHandleDataRequest lacks status code
>>>       if (master->serverState == fssHandleDataRequest)
>>>           originDataDownloadAbortedOnError = (originStatus > 400);
>>>
>>>       debugs(33, 5 "too early to write the response");
>>>       return;
>>> }
>>>
>>> if (delayedReply) {
>>>       writeForwardedReply(delayedReply.getRaw());
>>>       delayedReply = nullptr;
>>> } else {
>>>       completeDataExchange();
>>> }
>>
>> I did not follow your suggestion here.
>
> It might be easier to see in a simpler context. I believe Alex is
> suggestign you use a design closer to the stopReading()/stopWriting()
> methods in ConnStateData.
>   Each of those methods sets its end-of-X actions, then checks for Yn
> action already having finished. If both X and Y have finished, then
> schedules the finalization. Otherwise just exits the call chain.

Yep.
But:
   - the master->userDataDone is used only in fssHandleDataRequest case, 
and in all other cases will set to false causing bugs if moved out of an 
"if (fssHandleDataRequest)".
   - The completeDataExchange it is required in the fssHandleDataRequest 
case. Of course the suggested  block "if (delayedReply) {} else {}" will 
work and looks better. But just my opinion is that at least for now keep 
completeDataExchange into an "if(fssHandleDataRequest)" block. Else we 
need to rename completeDataExchange and change it a little.


>
>> The completeDataExchange should be called only if we are downloading
>> data. The exception case is the fssHandleDataRequest case so I am
>> feeling that it is better to have a block with this case.
>> But I adjusted the block.
>>
>> Also:
>>    - The master->userDataDone is adjusted now inside the
>> Ftp::Server::userDataCompletionCheckpoint, not inside
>> completeDataExchange method
>>   - I split my big commend to one comment inside
>> Ftp::Server::stopWaitingForOrigin and an other small comment inside
>> Ftp::Server::userDataCompletionCheckpoint where the master->userDataDone
>> is updated.
>>
>>
>> Probably the completeDataExchange should be renamed to
>> completeDataDownload. It is used only while we are downloading objects
>> (not uploading).
>>
>
> Agreed "exchange" sounds like a whole-transaction thing. Not a one-way
> completion.

yep.

>
>
>>>
>>> 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.
>>fssHandleDataRequest
>
> The below is an audit of the code comments and style only:
>
> * stopOriginWait() does not make sense in English.
>   - s/if waits/if originWaitInProgress/
>   - OR, s/if waits/if waiting for origin/

The correct looks that it is the "if originWaitInProgress"

>
>
> * double empty line after originWaitInProgress. please reduce to 1 line.

ok

>
>
> * readTransferDoneReply() its not clear what the moved debugs() is saying
>   - is it finishing use of a data channel?
>   - is it finished one object send/receive on a data channel that had (or
> will have) multiple transfers happening on it?
>   - is the new location of the debugs still agnostic to up/down direction
> of transfer?


You are right here. This method is called after one downloading (not 
uploading) is finished and we received from FTP server a control code 
about downloading status.
The data channel is always used for one object download/upload.

Is it ok to just add a debugs like the following:

+    debugs(9, 2, "Complete data downloading");


>
>
> I'm not going to +1 this right now because I'm not clear on what the
> logic should be. Consider this a 'no objection' from me.

I am waiting your and Alex comments before proceed.

>
>
> Amos