Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/15/2016 07:28 PM, Christos Tsantilas wrote: I applied the t4 patch to trunk as r14592 The t4 patch applied to squid-3.5 as r14001 I am attaching the t4 patch for squid-3.5. I believe that we should apply this patch for this major reasons: - The (statefull) FTP protocol requires good coordination between client-side and server side. The pinned connections is one mechanism which helps, but this is not enough. The stopWaiting/startWaiting adds one more mechanism. Will help us to solve more problems in the future. - The t4 patch also solves one more bug: In the case the FTP server respond with an error status after the download is finished, squid will return wrong status code to the user (success). Fixing this bug is possible because of the new mechanism. Christos On 03/15/2016 01:01 AM, Amos Jeffries wrote: On 15/03/2016 10:41 a.m., Alex Rousskov wrote: On 03/10/2016 02:35 PM, Alex Rousskov wrote: Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is enough to fix the known assertion. Take2 fixes that assertion as well, but it is bigger because it also fixes design problems that may lead to other bugs in v3.5. Which one do you want in v3.5? Amos, that question still stands: What do you want to see in v3.5? The simple take1 patch that only addresses a specific assertion failure OR the ported take4+ patch that attempts to fix the general flaw via the new startWaitingForOrigin()/stopWaitingForOrigin() mechanism? I dont have a preference for either one. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
I applied the t4 patch to trunk as r14592
I am attaching the t4 patch for squid-3.5.
I believe that we should apply this patch for this major reasons:
- The (statefull) FTP protocol requires good coordination between
client-side and server side. The pinned connections is one mechanism
which helps, but this is not enough. The stopWaiting/startWaiting adds
one more mechanism. Will help us to solve more problems in the future.
- The t4 patch also solves one more bug: In the case the FTP server
respond with an error status after the download is finished, squid will
return wrong status code to the user (success). Fixing this bug is
possible because of the new mechanism.
Christos
On 03/15/2016 01:01 AM, Amos Jeffries wrote:
On 15/03/2016 10:41 a.m., Alex Rousskov wrote:
On 03/10/2016 02:35 PM, Alex Rousskov wrote:
Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is
enough to fix the known assertion. Take2 fixes that assertion as well,
but it is bigger because it also fixes design problems that may lead to
other bugs in v3.5. Which one do you want in v3.5?
Amos, that question still stands: What do you want to see in v3.5? The
simple take1 patch that only addresses a specific assertion failure OR
the ported take4+ patch that attempts to fix the general flaw via the
new startWaitingForOrigin()/stopWaitingForOrigin() mechanism?
I dont have a preference for either one.
Amos
assertion failed: Write.cc:41: "!ccb->active()"
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This bug is an corner case, where the client-side (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.
This patch try to fix the existing mechanism as follows:
- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
writting possible responses to the client and keeps waiting for the
stopWaitingForOrigin callback
- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
resumes any postponed response.
- When the Ftp::Client starts working on a DATA-related transaction, calls the
Ftp::Server::startWaitingForOrigin callback
- When the Ftp::Client finishes its job or when its abort abnormaly, checks
whether it needs to call Ftp::Server::stopWaitingForOrigin callback.
- Also this patch try to fix the status code returned to the FTP client
taking in account the status code returned by FTP server. The
"Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
to the client side.
This is a Measurement Factory project
=== modified file 'src/clients/FtpRelay.cc'
--- src/clients/FtpRelay.cc 2016-01-31 06:14:05 +
+++ src/clients/FtpRelay.cc 2016-03-15 12:44:32 +
@@ -39,77 +39,84 @@
const Ftp::MasterState () const;
Ftp::MasterState ();
Ftp::ServerState serverState() const { return master().serverState; }
void serverState(const Ftp::ServerState newState);
/* Ftp::Client API */
virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
virtual void dataChannelConnected(const CommConnectCbParams );
/* Client API */
virtual void serverComplete();
virtual void handleControlReply();
virtual void processReplyBody();
virtual void handleRequestBodyProducerAborted();
virtual bool mayReadVirginReplyBody() const;
virtual void completeForwarding();
virtual bool abortOnData(const char *reason);
/* AsyncJob API */
virtual void start();
+virtual void swanSong();
void forwardReply();
void forwardError(err_type error = ERR_NONE, int xerrno = 0);
void failedErrorMessage(err_type error, int xerrno);
HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int64_t clen = 0);
void handleDataRequest();
void startDataDownload();
void startDataUpload();
bool startDirTracking();
void stopDirTracking();
bool weAreTrackingDir() const {return savedReply.message != NULL;}
typedef void (Relay::*PreliminaryCb)();
void forwardPreliminaryReply(const PreliminaryCb cb);
void proceedAfterPreliminaryReply();
PreliminaryCb thePreliminaryCb;
typedef void (Relay::*SM_FUNC)();
static const SM_FUNC SM_FUNCS[];
void readGreeting();
void sendCommand();
void readReply();
void readFeatReply();
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 15/03/2016 10:41 a.m., Alex Rousskov wrote: > On 03/10/2016 02:35 PM, Alex Rousskov wrote: >> Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is >> enough to fix the known assertion. Take2 fixes that assertion as well, >> but it is bigger because it also fixes design problems that may lead to >> other bugs in v3.5. Which one do you want in v3.5? > > > Amos, that question still stands: What do you want to see in v3.5? The > simple take1 patch that only addresses a specific assertion failure OR > the ported take4+ patch that attempts to fix the general flaw via the > new startWaitingForOrigin()/stopWaitingForOrigin() mechanism? > I dont have a preference for either one. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/10/2016 02:35 PM, Alex Rousskov wrote: > Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is > enough to fix the known assertion. Take2 fixes that assertion as well, > but it is bigger because it also fixes design problems that may lead to > other bugs in v3.5. Which one do you want in v3.5? Amos, that question still stands: What do you want to see in v3.5? The simple take1 patch that only addresses a specific assertion failure OR the ported take4+ patch that attempts to fix the general flaw via the new startWaitingForOrigin()/stopWaitingForOrigin() mechanism? Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
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.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/14/2016 06:33 PM, Alex Rousskov wrote:
On 03/13/2016 01:57 PM, Christos Tsantilas wrote:
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:
I did not follow your suggestion here.
The completeDataExchange should be called only if we are downloading
data.
I think your code is [still] correct but, thanks to your comments, I now
understand where [my] confusion about its meaning was coming from. I
suggest the following (functionally-equivalent) code:
void
Ftp::Server::stopWaitingForOrigin(int originStatus)
{
Must(waitingForOrigin);
waitingForOrigin = false;
// completeDataExchange() could be waitingForOrigin in fssHandleDataRequest
if (master->serverState == fssHandleDataRequest) {
// Depending on which side has finished downloading first, either trust
// master->userDataDone status or set originDataDownloadAbortedOnError:
if (master->userDataDone) {
// We finished downloading before Ftp::Client. Most likely, the
// adaptation shortened the origin response or we hit an error.
// Our status (stored in master->userDataDone) is more informative.
// Use master->userDataDone; avoid
originDataDownloadAbortedOnError.
completeDataExchange();
} else {
debugs(33, 5, "too early to write the response");
// Ftp::Client naturally finished downloading before us. Set
// originDataDownloadAbortedOnError to overwrite future
// master->userDataDone and relay Ftp::Client error, if there was
// any, to the user.
originDataDownloadAbortedOnError = (originStatus >= 400);
}
return;
}
if (delayedReply) {
writeForwardedReply(delayedReply.getRaw());
delayedReply = nullptr;
}
}
It is OK, fixed locally here.
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. 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)
The delayedReply can not be ignored because at least must be set to null.
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).
I have to admit that your proposal to forward the delayedReply if it is
set, else use completeDataExhange is the correct solution for this.
But we must check if we are in fssHandleDataRequest state before call
the completeDataExchange.
Why are we prioritizing completeDataExchange() over
writeForwardedReply(delayedReply)?
You are right. We must not do it.
And if we are doing the right thing, should we either assert that with
Must(!delayedReply) in the beginning of the fssHandleDataRequest clause
or clear the delayedReply, if any, in that clause?
+debugs(33, 5, "Transfering from FTP server is not complete");
+debugs(33, 5, "Transfering from FTP server terminated with an error, adjust
status code");
s/FTP server/FTP origin server/ to minimize the chance of this "FTP
server" phrase being misinterpreted as Ftp::Server.
OK on this
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?
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.
Please do, especially if my concerns regarding the combination of
delayedReply and fssHandleDataRequest state are
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/13/2016 10:17 PM, Amos Jeffries wrote: > * stopOriginWait() does not make sense in English. Not my expertise area, but it does make sense to me. "Wait" can be a noun as in "we had a long wait" (as suggested by Google). Would you prefer stopOriginWaiting() accompanied by s/originWaitInProgress/originWaiting/, despite the fact that this Ftp::Relay method terminates the wait experienced by another job (Ftp::Server) and not Ftp::Relay's own waiting? Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
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.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/13/2016 01:57 PM, Christos Tsantilas wrote:
> 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:
> I did not follow your suggestion here.
> The completeDataExchange should be called only if we are downloading
> data.
I think your code is [still] correct but, thanks to your comments, I now
understand where [my] confusion about its meaning was coming from. I
suggest the following (functionally-equivalent) code:
> void
> Ftp::Server::stopWaitingForOrigin(int originStatus)
> {
> Must(waitingForOrigin);
> waitingForOrigin = false;
>
> // completeDataExchange() could be waitingForOrigin in
> fssHandleDataRequest
> if (master->serverState == fssHandleDataRequest) {
> // Depending on which side has finished downloading first, either
> trust
> // master->userDataDone status or set
> originDataDownloadAbortedOnError:
> if (master->userDataDone) {
> // We finished downloading before Ftp::Client. Most likely, the
> // adaptation shortened the origin response or we hit an error.
> // Our status (stored in master->userDataDone) is more
> informative.
> // Use master->userDataDone; avoid
> originDataDownloadAbortedOnError.
> completeDataExchange();
> } else {
> debugs(33, 5, "too early to write the response");
> // Ftp::Client naturally finished downloading before us. Set
> // originDataDownloadAbortedOnError to overwrite future
> // master->userDataDone and relay Ftp::Client error, if there was
> // any, to the user.
> originDataDownloadAbortedOnError = (originStatus >= 400);
> }
> return;
> }
>
> if (delayedReply) {
> writeForwardedReply(delayedReply.getRaw());
> delayedReply = nullptr;
> }
> }
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?
Why are we prioritizing completeDataExchange() over
writeForwardedReply(delayedReply)?
And if we are doing the right thing, should we either assert that with
Must(!delayedReply) in the beginning of the fssHandleDataRequest clause
or clear the delayedReply, if any, in that clause?
> +debugs(33, 5, "Transfering from FTP server is not complete");
> +debugs(33, 5, "Transfering from FTP server terminated with an error,
> adjust status code");
s/FTP server/FTP origin server/ to minimize the chance of this "FTP
server" phrase being misinterpreted as Ftp::Server.
> Probably the completeDataExchange should be renamed to
> completeDataDownload. It is used only while we are downloading objects
> (not uploading).
Agreed.
>> 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.
Please do, especially if my concerns regarding the combination of
delayedReply and fssHandleDataRequest state are easily addressed.
Thank you,
Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
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
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
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
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
I would like to respond on some of the things in the post related to V4.
First 3.5 is very stable for a very long time now.
It have couple bugs in it and it will be good to somehow make them gone
but I must highlight couple things which might not be clear.
Atomic patches are great and they solve things in a very good way.
But every bug has it's own "life" and we can understand that it would
not be possible to write a perfect product without couple bugs\errors\typos.
I like many believe in perfection but from the last test(RAM ONLY) I
ran, it seems that the hardware is not really ready to specific heat.
The DDR2 ECC cards started pooping out of their socket in the middle of
a simple test when it's pretty cold in the room...
+1 for trunk while I still have couple doubts about what I am
understanding about take1 and take2 steps.
I will try to see how the service works after the patches will be
applied to V4.
I have couple more build nodes and the automation learning keeps me busy.
Eliezer
On 10/03/2016 23:35, Alex Rousskov wrote:
On 03/10/2016 12:14 PM, Christos Tsantilas wrote:
I am attaching two patches for this bug.
I will re-summarize the problem we are dealing with using higher-level
concepts so that it is easier to grok what Christos is talking about:
1. Ftp::Client cannot deal with more than one FTP command at a time.
2. Ftp::Server must _delay_ reading the next FTP command due to #1.
3. Delaying is complicated because Ftp::Server receives responses from
"Squid" (ICAP, errors, caching, etc.), and not Ftp::Client itself:
Ftp:Server may receive a response for the current FTP command
long _before_ Ftp::Client is done dealing with that same command!
Christos has highlighted a few specific cases where that happens.
[ In HTTP, this problem is much smaller because the two Squid HTTP
sides do not share much state: New Http::Client objects are created
as needed for the same ConnStateData object while the old ones
are finishing their Http::Client jobs. ]
4. The current Squid code (v3.5 and trunk) contains a mechanism for #2,
but that mechanism does not handle a now-known race condition. The
bug leads to the Write.cc assertion. Our take1 patch fixes that bug.
5. While working on #4, we realized that the existing mechanism for #2
relies on Ftp::Client existence and other conditions that may not
materialize. If things go wrong, Ftp::Server may get stuck waiting
for Ftp::Client that either did not exist or did not end the wait.
Our take2 patch revises the mechanism for #2 to be much more robust.
We do not know of any specific way to hit the bugs fixed by take2,
but that does not mean it is (and will remain) impossible.
Take2 also relays FTP origin errors to the FTP user in more cases.
One simple for squid-3.5 (t1
patch) and one more complex (t2 patch). The simple patch solve the bug
for now, but may leave other similar bugs in squid.
Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is
enough to fix the known assertion. Take2 fixes that assertion as well,
but it is bigger because it also fixes design problems that may lead to
other bugs in v3.5. Which one do you want in v3.5?
For trunk/v4, there is no doubt in my mind that we should use take2 (or
its polished variant).
The rest is my take2 review.
+bool clientSideWaitingForUs; ///< whether the client is waiting for us
Let's avoid confusing "client side" and "client" terms in new code,
especially when it is Ftp::Server that is waiting:
/// whether we are between Ftp::Server::startWaitingForOrigin() and
/// Ftp::Server::stopWaitingForOrigin() calls
bool originWaitInProgress;
+void
+Ftp::Relay::swanSong()
+{
+if (clientSideWaitingForUs) {
+CbcPointer = fwd->request->clientConnectionManager;
+if (mgr.valid()) {
+if (Ftp::Server *srv = dynamic_cast(mgr.get())) {
+typedef UnaryMemFunT CbDialer;
+AsyncCall::Pointer call = asyncCall(11, 3,
"Ftp::Server::stopWaitingForOrigin",
+CbDialer(srv,
::Server::stopWaitingForOrigin, 0));
+ScheduleCallHere(call);
and
void
Ftp::Relay::serverComplete()
{
CbcPointer = fwd->request->clientConnectionManager;
if (mgr.valid()) {
+if (clientSideWaitingForUs) {
+if (Ftp::Server *srv = dynamic_cast(mgr.get())) {
+ typedef UnaryMemFunT CbDialer;
+ AsyncCall::Pointer call = asyncCall(11, 3,
"Ftp::Server::stopWaitingForOrigin",
+ CbDialer(srv,
::Server::stopWaitingForOrigin, ctrl.replycode));
+ ScheduleCallHere(call);
+ clientSideWaitingForUs = false;
+}
+}
Let's move this nearly duplicated code into a new
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
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.
assertion failed: Write.cc:41: "!ccb->active()"
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This bug is an corner case, where the client-side (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.
This patch try to fix the existing mechanism as follows:
- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
writting possible responses to the client and keeps waiting for the
stopWaitingForOrigin callback
- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
resumes any postponed response.
- When the Ftp::Client starts working on a DATA-related transaction, calls the
Ftp::Server::startWaitingForOrigin callback
- When the Ftp::Client finishes its job or when its abort abnormaly, checks
whether it needs to call Ftp::Server::stopWaitingForOrigin callback.
- Also this patch try to fix the status code returned to the FTP client
taking in account the status code returned by FTP server. The
"Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
to the client side.
This is a Measurement Factory project
=== modified file 'src/clients/FtpRelay.cc'
--- src/clients/FtpRelay.cc 2016-03-13 04:47:19 +
+++ src/clients/FtpRelay.cc 2016-03-13 19:27:49 +
@@ -42,77 +42,85 @@
const Ftp::MasterState () const;
Ftp::MasterState ();
Ftp::ServerState serverState() const { return master().serverState; }
void serverState(const Ftp::ServerState newState);
/* Ftp::Client API */
virtual void failed(err_type error = ERR_NONE, int xerrno = 0, ErrorState *ftperr = nullptr);
virtual void dataChannelConnected(const CommConnectCbParams );
/* Client API */
virtual void serverComplete();
virtual void handleControlReply();
virtual void processReplyBody();
virtual void handleRequestBodyProducerAborted();
virtual bool mayReadVirginReplyBody() const;
virtual
Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
On 03/10/2016 12:14 PM, Christos Tsantilas wrote:
> I am attaching two patches for this bug.
I will re-summarize the problem we are dealing with using higher-level
concepts so that it is easier to grok what Christos is talking about:
1. Ftp::Client cannot deal with more than one FTP command at a time.
2. Ftp::Server must _delay_ reading the next FTP command due to #1.
3. Delaying is complicated because Ftp::Server receives responses from
"Squid" (ICAP, errors, caching, etc.), and not Ftp::Client itself:
Ftp:Server may receive a response for the current FTP command
long _before_ Ftp::Client is done dealing with that same command!
Christos has highlighted a few specific cases where that happens.
[ In HTTP, this problem is much smaller because the two Squid HTTP
sides do not share much state: New Http::Client objects are created
as needed for the same ConnStateData object while the old ones
are finishing their Http::Client jobs. ]
4. The current Squid code (v3.5 and trunk) contains a mechanism for #2,
but that mechanism does not handle a now-known race condition. The
bug leads to the Write.cc assertion. Our take1 patch fixes that bug.
5. While working on #4, we realized that the existing mechanism for #2
relies on Ftp::Client existence and other conditions that may not
materialize. If things go wrong, Ftp::Server may get stuck waiting
for Ftp::Client that either did not exist or did not end the wait.
Our take2 patch revises the mechanism for #2 to be much more robust.
We do not know of any specific way to hit the bugs fixed by take2,
but that does not mean it is (and will remain) impossible.
Take2 also relays FTP origin errors to the FTP user in more cases.
> One simple for squid-3.5 (t1
> patch) and one more complex (t2 patch). The simple patch solve the bug
> for now, but may leave other similar bugs in squid.
Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is
enough to fix the known assertion. Take2 fixes that assertion as well,
but it is bigger because it also fixes design problems that may lead to
other bugs in v3.5. Which one do you want in v3.5?
For trunk/v4, there is no doubt in my mind that we should use take2 (or
its polished variant).
The rest is my take2 review.
> +bool clientSideWaitingForUs; ///< whether the client is waiting for us
Let's avoid confusing "client side" and "client" terms in new code,
especially when it is Ftp::Server that is waiting:
/// whether we are between Ftp::Server::startWaitingForOrigin() and
/// Ftp::Server::stopWaitingForOrigin() calls
bool originWaitInProgress;
> +void
> +Ftp::Relay::swanSong()
> +{
> +if (clientSideWaitingForUs) {
> +CbcPointer =
> fwd->request->clientConnectionManager;
> +if (mgr.valid()) {
> +if (Ftp::Server *srv = dynamic_cast(mgr.get())) {
> +typedef UnaryMemFunT CbDialer;
> +AsyncCall::Pointer call = asyncCall(11, 3,
> "Ftp::Server::stopWaitingForOrigin",
> +CbDialer(srv,
> ::Server::stopWaitingForOrigin, 0));
> +ScheduleCallHere(call);
and
> void
> Ftp::Relay::serverComplete()
> {
> CbcPointer = fwd->request->clientConnectionManager;
> if (mgr.valid()) {
> +if (clientSideWaitingForUs) {
> +if (Ftp::Server *srv = dynamic_cast(mgr.get())) {
> + typedef UnaryMemFunT CbDialer;
> + AsyncCall::Pointer call = asyncCall(11, 3,
> "Ftp::Server::stopWaitingForOrigin",
> + CbDialer(srv,
> ::Server::stopWaitingForOrigin, ctrl.replycode));
> + ScheduleCallHere(call);
> + clientSideWaitingForUs = false;
> +}
> +}
Let's move this nearly duplicated code into a new
Ftp::Relay::stopOriginWait(int code) method _and_ always clear
clientSideWaitingForUs/originWaitInProgress there.
> void
> Ftp::Server::writeForwardedReply(const HttpReply *reply)
> {
> Must(reply);
>
> +if (master->waitingForOrigin) {
> +delayedReply = reply;
> +return;
> +}
> +
If this method is called twice while waitingForOrigin is true, should we
throw an exception, honor the first reply, or honor the last one? Please
adjust the code as need. *If* honoring the last reply is the best
strategy (which is what take2 does), add a comment:
delayedReply = reply; // and forget any previous ones
> +debugs(33, 5, "Transfering data in progress, waitting server side to
> finish");
I suggest using "waiting for Ftp::Client data transfer to end" for
brevity sake. This will also fix the two spelling errors in the above text.
> +// Currently only data upload and data download requests are
> +// waiting for the origin:
> +// Must(master->serverState == fssHandleUploadRequest ||
> +
[squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"
Hi all,
I am attaching two patches for this bug. One simple for squid-3.5 (t1
patch) and one more complex (t2 patch). The simple patch solve the bug
for now, but may leave other similar bugs in squid.
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This bug is an corner case, where the client-side (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job
before respond to the FTP client. In this bug the existing mechanism,
designed to handle such problems, did not worked correctly and resulted
to a double writing to the client.
To understand why it is important such mechanism, imagine the case where
an FTP client download a huge file and the ICAP server
block this file. In this case Squid needs to delay response to the FTP
client until the squid server-side closes its data connection too and
get the response from FTP server. This is required because we can not
accept more commands from FTP client, if we can not forward them to the
FTP server.
Moreover Squid can fall into such cases when a download or upload may
aborted abnormally by one of the client or server. The problem can
become more complicated if ICAP/adaptation is used and squid respond to
a download request without contacting the FTP server.
This patch try to fix the existing mechanism as follows:
- When Ftp::Server receives a "startWaitingForOrigin" callback,
postpones writting possible responses to the client and keeps waiting
for the stopWaitingForOrigin callback
- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
resumes any postponed response.
- When the Ftp::Client starts working on a DATA-related transaction
calls the Ftp::Server::startWaitingForOrigin callback
- When the Ftp::Client finishes its job or when its abort abnormaly,
checks whether it needs to call Ftp::Server::stopWaitingForOrigin callback.
- Also this patch try to fix the status code returned to the FTP client
taking in account the status code returned by FTP server. The
"Ftp::Server::stopWaitingForOrigin" is used to pass the returned status
code to the client side.
This is a Measurement Factory project
assertion failed: Write.cc:41: "!ccb->active()"
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This patch:
- Sets the Ftp::Server::master::waitForOriginData flag to false inside
Ftp::Server::originDataCompletionCheckpoint(), justs before writes the
reply message to the client, instead of setting it inside
Ftp::Relay::finalizeDataDownload(), to avoid call twice the
Ftp::Server::completeDataExchange method id both
originDataCompletionCheckpoint and userDataCompletionCheckpoint called
concurrently
- Does not sets the Ftp::Server::master->waitForOriginData to false inside
Ftp::Server::userDataCompletionCheckpoint method if the server side
connection is closed, to avoid crash in the case the server side
schedules the originDataCompletionCheckpoint before closed the server
side connection.
In this case squid will close the client-side connection too, because
it is pinned with the server-side connection. We do not have to
worry for no answer to the ftp-client.
This is a Measurement Factory project
=== modified file 'src/clients/FtpRelay.cc'
--- src/clients/FtpRelay.cc 2016-01-31 06:14:05 +
+++ src/clients/FtpRelay.cc 2016-03-10 18:17:31 +
@@ -699,42 +699,40 @@
}
debugs(9, 2, "connected FTP server data channel: " << io.conn);
data.opened(io.conn, dataCloser());
sendCommand();
}
void
Ftp::Relay::scheduleReadControlReply()
{
Ftp::Client::scheduleReadControlReply(0);
}
void
Ftp::Relay::finalizeDataDownload()
{
debugs(9, 2, "Complete data downloading/Uploading");
-updateMaster().waitForOriginData = false;
-
CbcPointer = fwd->request->clientConnectionManager;
if (mgr.valid()) {
if (Ftp::Server *srv = dynamic_cast(mgr.get())) {
typedef NullaryMemFunT CbDialer;
AsyncCall::Pointer call = JobCallback(11, 3,
