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

[Alex Rousskov]

On 03/13/2016 10:17 PM, Amos Jeffries wrote:
> On 14/03/2016 8:57 a.m., Christos Tsantilas wrote:
>> On 03/10/2016 11:35 PM, Alex Rousskov wrote:
>>> 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.


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

That part is already implemented in Christos patch and does not need
fixing AFAICT: userDataDone and waitingForOrigin are indeed somewhat
similar to reading/writing states in ConnStateData. The situation is
more complex in FTP code because the states are not as universal as
reading and writing -- the states and their correct interpretation
depends on the FTP command being processed. My dissatisfaction was not
with this mechanism though.

My comment was about how we react to [one of] the "stop" event[s]. My
alternative sketch was bad though because I misunderstood what exactly
Christos was trying to accomplish. The comments on this thread helped me
arrive at the [hopefully] final suggestion that I just posted in
response to his take3 patch.


> * 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 think it is #1 and #3. The Ftp::Relay code does not reuse data
connections IIRC -- dataComplete() is called after each data transfer. I
am not even sure FTP allows for data connection reuse (but that
discussion is outside this thread scope).

I do not see a much better way to phrase that debugs(), and we should
not demand a change to the old debugs() line that was simply moved to
the [only] caller (AFAICT). Said that, if I were to polish this, I would
merge the moved debugs() with the status-only debugs() already present
in the beginning of this method:

 void
 Ftp::Relay::readTransferDoneReply()
 {
    debugs(9, 2, "origin confirmed download/upload end with FTP " <<
status());
    ...


s/confirmed/signaled/ if you prefer.


Thank you,

Alex.