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

[Christos Tsantilas]

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

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


>
>
> Thank you,
>
> Alex.
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-156-assertion_failed_Write.cc_41-not_ccb_active-trunk-t3.patch
Type: text/x-patch
Size: 25904 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160313/e017fb58/attachment-0001.bin>