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

[Alex Rousskov]

On 03/14/2016 02:58 PM, Christos Tsantilas wrote:
> On 03/14/2016 06:33 PM, Alex Rousskov wrote:
>> 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.

You are probably right, but I would not be surprised if there are (or
will be) other conditions (e.g., the error reply coming from the ICAP
service rather than Ftp::Client itself). My point is not that we should
research and enumerate all possible cases, but that the code should
handle all of them (whatever they are) in a general fashion. Take3 does
not quite do that as discussed above.

To avoid snowballing this into an endless Ftp::* rewrite, I would limit
related changes to making Ftp::Server::stopWaitingForOrigin() do the
right thing unless you know of other _specific_ methods/cases where our
code should be changed to accommodate the new delayedReply mechanism.


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

... which is something too fragile to rely on, as you seem to be
implying as well.


> The delayedReply can not be ignored because at least must be set to null.

To avoid Must() failures later? Glad I asked then.


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

In general, forwarding the _first_ [error] response is the best strategy
if we might be dealing with multiple error sources/responses.


> I have to admit that your proposal to forward the delayedReply if it is
> set, else use completeDataExhange is the correct solution for this.

Perhaps we can do that before the fssHandleDataRequest check? Like this:

Ftp::Server::stopWaitingForOrigin(int originStatus)
{
   Must(waitingForOrigin);
   waitingForOrigin = false;

   // if we have already decided how to respond, respond now
   if (delayedReply) {
       writeForwardedReply(delayedReply.getRaw());
       delayedReply = nullptr;
       return; // do not completeDataExchange() after an earlier response
   }

   if (master->serverState != fssHandleDataRequest)
       return;

   ... our fssHandleDataRequest handling code here ...
   ... including a conditional completeDataExchange() call ...


BTW, I would recommend resetting this->delayedReply _before_ calling
writeForwardedReply() with that reply object for the fear of reentrant
calls discovering this->delayedReply still set... You will need a
temporary/local HttpReply::Pointer (not a raw pointer!) for that to work.


> But we must check if we are in fssHandleDataRequest state before call
> the completeDataExchange.


Yes, that does not change.


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

Sure. It clarifies our code quite a bit IMO. Please commit ASAP, when
you are comfortable with the result.


Thank you,

Alex.