date:20160313

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

2016-03-13 Thread 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

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Build failed in Jenkins: trunk-full-matrix » clang,d-debian-jessie #125

2016-03-13 Thread noc

See 


--
[...truncated 36365 lines...]
mv -f $depbase.Tpo $depbase.Po
depbase=`echo DelayUser.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
ccache clang++ -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"
 
-DDEFAULT_SQUID_DATA_DIR=\"
 
-DDEFAULT_SQUID_CONFIG_DIR=\"
   -I../.. -I../../include -I../../lib -I../../src -I../include-I../src   
-I/usr/include/libxml2  -I/usr/include/libxml2  -Werror -Qunused-arguments 
-Wno-deprecated-register  -D_REENTRANT -g -O2 -std=c++11 -MT DelayUser.o -MD 
-MP -MF $depbase.Tpo -c -o DelayUser.o ../../src/DelayUser.cc &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo DelayVector.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
ccache clang++ -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"
 
-DDEFAULT_SQUID_DATA_DIR=\"
 
-DDEFAULT_SQUID_CONFIG_DIR=\"
   -I../.. -I../../include -I../../lib -I../../src -I../include-I../src   
-I/usr/include/libxml2  -I/usr/include/libxml2  -Werror -Qunused-arguments 
-Wno-deprecated-register  -D_REENTRANT -g -O2 -std=c++11 -MT DelayVector.o -MD 
-MP -MF $depbase.Tpo -c -o DelayVector.o ../../src/DelayVector.cc &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo NullDelayId.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
ccache clang++ -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"
 
-DDEFAULT_SQUID_DATA_DIR=\"
 
-DDEFAULT_SQUID_CONFIG_DIR=\"
   -I../.. -I../../include -I../../lib -I../../src -I../include-I../src   
-I/usr/include/libxml2  -I/usr/include/libxml2  -Werror -Qunused-arguments 
-Wno-deprecated-register  -D_REENTRANT -g -O2 -std=c++11 -MT NullDelayId.o -MD 
-MP -MF $depbase.Tpo -c -o NullDelayId.o ../../src/NullDelayId.cc &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ClientDelayConfig.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
ccache clang++ -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"
 
-DDEFAULT_SQUID_DATA_DIR=\"
 
-DDEFAULT_SQUID_CONFIG_DIR=\"
   -I../.. -I../../include -I../../lib -I../../src -I../include-I../src   
-I/usr/include/libxml2  -I/usr/include/libxml2  -Werror -Qunused-arguments 
-Wno-deprecated-register  -D_REENTRANT -g -O2 -std=c++11 -MT 
ClientDelayConfig.o -MD -MP -MF $depbase.Tpo -c -o ClientDelayConfig.o 
../../src/ClientDelayConfig.cc &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo fs_io.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
ccache clang++ -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"
 
-DDEFAULT_SQUID_DATA_DIR=\"
 
-DDEFAULT_SQUID_CONFIG_DIR=\"
   -I../.. -I../../include -I../../lib -I../../src -I../include-I../src   
-I/usr/include/libxml2  -I/usr/include/libxml2  -Werror -Qunused-arguments 
-Wno-deprecated-register  -D_REENTRANT -g -O2 -std=c++11 -MT fs_io.o -MD -MP 
-MF $depbase.Tpo -c -o fs_io.o ../../src/fs_io.cc &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo dlink.o | sed

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

2016-03-13 Thread Eliezer Croitoru


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

[squid-dev] Build failed in Jenkins: trunk-full-matrix » clang,d-debian-unstable #125

2016-03-13 Thread noc

See 


--
[...truncated 6843 lines...]
make[5]: Leaving directory 
'
Making dvi in RADIUS
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in SMB
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in SMB_LM
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in fake
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in getpwnam
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in digest
make[4]: Entering directory 
'
Making dvi in file
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in negotiate
make[4]: Entering directory 
'
Making dvi in wrapper
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory

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

2016-03-13 Thread 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.




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

[squid-dev] Build failed in Jenkins: trunk-full-matrix » clang,d-debian-jessie #125

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

[squid-dev] Build failed in Jenkins: trunk-full-matrix » clang,d-debian-unstable #125

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

5 matches

Site Navigation

Mail list logo

Footer information