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

[Amos Jeffries]

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 Y
action already having finished. If both X and Y have finished, then
schedules the finalization. Otherwise just exits the call chain.


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


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

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/


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


* 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?


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.


Amos