Re: squid-3 icap related patch

2006-10-03 Thread Adrian Chadd 
On Wed, Oct 04, 2006, Tsantilas Christos wrote:
> Hi,
>  I can not reproduce the problem any more using the
> squid3-icap sources.
> I am testing for some hours the icap client and looks OK.
> I believe that you can ignore this patch.

Thanks a lot for your ICAP feedback, and thanks to Alex for importing
all the fixes into Squid-3.

Some of us were worried we'd have to learn ICAP to make it all
work before the release :)




Adrian

Re: squid-3 icap related patch

2006-10-03 Thread Tsantilas Christos 
Hi,
 I can not reproduce the problem any more using the
squid3-icap sources.
I am testing for some hours the icap client and looks OK.
I believe that you can ignore this patch.

Regards,
Christos

Alex Rousskov wrote:
> On Tue, 2006-10-03 at 08:48 -0600, Alex Rousskov wrote:
>> On Tue, 2006-10-03 at 11:36 +0300, Tsantilas Christos wrote:
>>
>>> I believe that the problem here is not the zero sized content.
>>> Squid calls this function when read all headers.
>>> There are cases in which  squids did not read any part of body yet
>>> .
> I cannot reproduce the above. I made a server that responds to an HTTP
> GET with HTTP headers and then waits before sending the message body.
> > 
>

Re: squid-3 icap related patch

2006-10-03 Thread Alex Rousskov 
On Tue, 2006-10-03 at 08:48 -0600, Alex Rousskov wrote:
> On Tue, 2006-10-03 at 11:36 +0300, Tsantilas Christos wrote:
>
> > I believe that the problem here is not the zero sized content.
> > Squid calls this function when read all headers.
> > There are cases in which  squids did not read any part of body yet
> > (and there is not Content-Lenght header so preview is disabled).
> > In this case squid sends to the icap server the "0\r\n\r\n" string
> > (0\r\n definition + \r\n after chunk)
> > The icap server believes that this is the end of body and does not
> > expect  more data.
> 
> I see! Writing last-chunk when the body is [still] expected is obviously
> wrong and should be fixed. I will try to reproduce and fix this.

I cannot reproduce the above. I made a server that responds to an HTTP
GET with HTTP headers and then waits before sending the message body.
There is no Content-Length header. After Squid received the HTTP
response headers and successfully wrote the ICAP message prefix to the
ICAP server, I get:

| ICAPModXact::noteCommWrote called [Comm(13wr)w(2)/]
| ICAP/ICAPModXact.cc(164) Wrote 670 bytes
| ICAP/ICAPModXact.cc(258) will write up to 0 bytes of prime virgin body
| ICAPModXact has no writeable prime virgin body content
| ICAP/ICAPModXact.cc(287) will write 0 raw bytes of prime virgin body
| ICAPModXact::noteCommWrote ended [Comm(13r)w(5)/]

As you can see, ICAPModXact refuses to write anything when there is no
response body yet. Was your test case different?

Thank you,

Alex.

Re: squid-3 icap related patch

2006-10-03 Thread Alex Rousskov 
On Tue, 2006-10-03 at 11:36 +0300, Tsantilas Christos wrote:
   
> Alex Rousskov wrote:
> >I believe zero size content is valid. If writeMore() cannot handle it,
> >writeMore() should be fixed and still called unconditionally in the
> >above code.

> I believe that the problem here is not the zero sized content.
> Squid calls this function when read all headers.
> There are cases in which  squids did not read any part of body yet
> (and there is not Content-Lenght header so preview is disabled).
> In this case squid sends to the icap server the "0\r\n\r\n" string
> (0\r\n definition + \r\n after chunk)
> The icap server believes that this is the end of body and does not
> expect  more data.

I see! Writing last-chunk when the body is [still] expected is obviously
wrong and should be fixed. I will try to reproduce and fix this.

In general, I am worried about adding a "guard" for writeMore() outside
of writeMore() because writeMore() is called from several places, on
several events.

Thank you,

Alex.

Re: squid-3 icap related patch

2006-10-03 Thread Tsantilas Christos 

Hi Alex,
  
Alex Rousskov wrote:



On Thu, 2006-09-28 at 05:18 -0400, Tsantilas Christos wrote:

 


3) File ICAP/ICAPModXact.cc function
ICAPModXact::handleCommWroteHeaders()

Sometimes, we have read the headers but not any part of the body yet
so in this case we must not call writeMore() function.

-writeMore();
+   if(virgin->data->body && virgin->data->body->contentSize())
+   writeMore();
   



As far as I can see, virgin->data->body is never NULL at this stage and
the rest of the code assumes it is not NULL. Thus, I assume the first
part of the above if-statement can be safely removed. This leaves us
with the zero-size content part.
 


Yes that the  virgin->data->body is never NULL. I will check it more...


I believe zero size content is valid. If writeMore() cannot handle it,
writeMore() should be fixed and still called unconditionally in the
above code. One place where zero-sized content is useful is for
zero-size previews, which are common (please see below on that).
 


I believe that the problem here is not the zero sized content.
Squid calls this function when read all headers.
There are cases in which  squids did not read any part of body yet
(and there is not Content-Lenght header so preview is disabled).
In this case squid sends to the icap server the "0\r\n\r\n" string
(0\r\n definition + \r\n after chunk)
The icap server believes that this is the end of body and does not
expect  more data.


Zero-size Preview is very useful because it allows an ICAP server to

receive the HTTP headers without the body before making any decisions.
Why do you want to disable it? Do we have zero-preview handling bugs
elsewhere?

 


You are right about zero-preview and zero size content.
Give me some time to check again if there is any zero-preview
handling bug.


I believe I have applied all other changes you wrote about. The changes
currently live on the squid3-icap branch in the SourceForge Squid
...
 


OK, I will use the squid3-icap branch for my tests.

Regards,
 Christos

Re: squid-3 icap related patch

2006-10-02 Thread Alex Rousskov 
On Thu, 2006-09-28 at 05:18 -0400, Tsantilas Christos wrote:

> 3) File ICAP/ICAPModXact.cc function
> ICAPModXact::handleCommWroteHeaders()
> 
> Sometimes, we have read the headers but not any part of the body yet
> so in this case we must not call writeMore() function.
> 
> -writeMore();
> +   if(virgin->data->body && virgin->data->body->contentSize())
> +   writeMore();

As far as I can see, virgin->data->body is never NULL at this stage and
the rest of the code assumes it is not NULL. Thus, I assume the first
part of the above if-statement can be safely removed. This leaves us
with the zero-size content part.

I believe zero size content is valid. If writeMore() cannot handle it,
writeMore() should be fixed and still called unconditionally in the
above code. One place where zero-sized content is useful is for
zero-size previews, which are common (please see below on that).


> 7) File ICAP/ICAPModXact.cc function ICAPModXact::estimateVirginBody
> 
> if function header->expectingBody(method, size) returns size==0 then
> we do not expect a body.

Can you explain why zero-size body should be treated differently here? I
am especially worried about the case were Squid may tell the ICAP server
that there was no HTTP entity body where in fact there was a body of
zero length. The difference is likely to affect the adaptation logic on
the ICAP server if the server adapts (or preserves) HTTP bodies.

Again, if zero-size bodies cause problems elsewhere in the ICAP code, we
should fix those problems, but hopefully without completely ignoring
such bodies.


> 8) File ICAP/ICAPModXact.cc function ICAPPreview::enable
> 
> The theAd is the expected preview size so if theAd==0 do not
> enable preview mode.
> 
> -theState = stWriting;
> +if(theAd >0)
> +   theState = stWriting;
> +else
> +   theState = stDisabled;

Zero-size Preview is very useful because it allows an ICAP server to
receive the HTTP headers without the body before making any decisions.
Why do you want to disable it? Do we have zero-preview handling bugs
elsewhere?


I believe I have applied all other changes you wrote about. The changes
currently live on the squid3-icap branch in the SourceForge Squid
repository. There are other ICAP bugs that must be fixed before ICAP
code can be declared stable. I hope to nail most in the next week or
so. 

I would appreciate if you can answer the above questions and/or provide
your test cases so that we can "close" the three pending changes one way
or the other.


Thank you,

Alex.

Re: squid-3 icap related patch

2006-10-02 Thread Alex Rousskov 
On Thu, 2006-09-28 at 05:18 -0400, Tsantilas Christos wrote:
> Hi all,
> 
> I am sending a small patch for icap client.
> With this patch squid-icap works for me (using 1-2 clients).
> I will test it more under load, using different scenarios.

I am working on committing yours and other ICAP fixes to CVS. My goal is
to make Squid3/ICAP pass Polygraph and real-world tests in a week or so.
If you find other ICAP bugs, please post them here or add them to
bugzilla.

Thanks a lot for your patch,

Alex.

squid-3 icap related patch

2006-09-28 Thread Tsantilas Christos 
Hi all,

I am sending a small patch for icap client.
With this patch squid-icap works for me (using 1-2 clients).
I will test it more under load, using different scenarios.

At the following lines I am trying to explain my changes:

1) File HttpReply.cc in function HttpReply::reset

pstate need to initialized again to psReadyToParseStartLine


2) Files http.c and ftp.c functions HttpStateData::doneAdapting,
HttpStateData::abortAdapting, FtpStateData::doneAdapting and
FtpStateData::abortAdapting

I comment out cbdataReferenceDone function call. I did not found any
cbdataReferenceValid call or something similar so I believe I am correct
here.


3) File ICAP/ICAPModXact.cc function ICAPModXact::handleCommWroteHeaders()

Sometimes, we have read the headers but not any part of the body yet so
in this case we must not call writeMore() function.

-writeMore();
+   if(virgin->data->body && virgin->data->body->contentSize())
+   writeMore();


4) File ICAP/ICAPModXact.cc function ICAPModXact::moveRequestChunk

The preview.wrote(chunkSize, state.doneReceiving) statement is not correct,
becouse  we can have all the data but we did not send all the data yet.

My patch is:

+const bool done = state.doneReceiving && claimSize(virginWriteClaim)
<= 0;
 if (state.writing == State::writingPreview)
-preview.wrote(chunkSize, state.doneReceiving); // even if wrote
nothing
+   preview.wrote(chunkSize, done); // even if wrote nothing


5) File ICAP/ICAPModXact.cc function ICAPModXact::addLastRequestChunk

The last chunk was not correct ("0\r\n; ieof\r\n" instead of "0;
ieof\r\n\r\n" ).


6) File ICAP/ICAPModXact.cc function ICAPModXact::handle100Continue

This function called when we read "100 Continue" responce from icap server.
After that we expect to read the real ICAP responce (ICAP headers
followed by http headers and http body).
In current implementation squid ICAP client expects to read the http headers.
The solution is:
-state.parsing = State::psHttpHeader; // eventually
+state.parsing = State::psIcapHeader;
+icapReply->reset();


7) File ICAP/ICAPModXact.cc function ICAPModXact::estimateVirginBody

if function header->expectingBody(method, size) returns size==0 then
we do not expect a body.


8) File ICAP/ICAPModXact.cc function ICAPPreview::enable

The theAd is the expected preview size so if theAd==0 do not
enable preview mode.

-theState = stWriting;
+if(theAd >0)
+   theState = stWriting;
+else
+   theState = stDisabled;


Index: HttpReply.cc
===
RCS file: /squid/squid3/src/HttpReply.cc,v
retrieving revision 1.89
diff -u -r1.89 HttpReply.cc
--- HttpReply.cc7 Jun 2006 22:39:33 -   1.89
+++ HttpReply.cc27 Sep 2006 12:25:29 -
@@ -105,6 +105,7 @@
 // are allowed with MEMPROXY_CLASS() and whether some cbdata void*
 // conversions are not going to kill virtual tables
 const String pfx = protoPrefix;
+pstate=psReadyToParseStartLine;
 clean();
 init();
 protoPrefix = pfx;
Index: ftp.cc
===
RCS file: /squid/squid3/src/ftp.cc,v
retrieving revision 1.407
diff -u -r1.407 ftp.cc
--- ftp.cc  19 Sep 2006 07:56:57 -  1.407
+++ ftp.cc  27 Sep 2006 12:25:43 -
@@ -3439,7 +3439,7 @@
  */
 delete icap;
 
-cbdataReferenceDone(icap);
+//cbdataReferenceDone(icap);
 
 if (ctrl.fd >= 0)
 comm_close(ctrl.fd);
@@ -3456,7 +3456,7 @@
  * ICAP has given up, we're done with it too
  */
 delete icap;
-cbdataReferenceDone(icap);
+//cbdataReferenceDone(icap);
 
 if (entry->isEmpty()) {
 ErrorState *err;
Index: http.cc
===
RCS file: /squid/squid3/src/http.cc,v
retrieving revision 1.507
diff -u -r1.507 http.cc
--- http.cc 20 Sep 2006 06:29:10 -  1.507
+++ http.cc 27 Sep 2006 12:25:47 -
@@ -2077,8 +2077,8 @@
  * ICAP is done, so we don't need this any more.
  */
 delete icap;
-
-cbdataReferenceDone(icap);
+icap = NULL;
+//cbdataReferenceDone(icap);
 
 if (fd >= 0)
 comm_close(fd);
@@ -2095,7 +2095,8 @@
  * ICAP has given up, we're done with it too
  */
 delete icap;
-cbdataReferenceDone(icap);
+icap = NULL;
+//cbdataReferenceDone(icap);
 
 if (entry->isEmpty()) {
 ErrorState *err;
Index: ICAP/ICAPModXact.cc
===
RCS file: /squid/squid3/src/ICAP/ICAPModXact.cc,v
retrieving revision 1.24
diff -u -r1.24 ICAPModXact.cc
--- ICAP/ICAPModXact.cc 8 May 2006 23:38:34 -   1.24
+++ ICAP/ICAPModXact.cc 27 Sep 2006 12:25:48 -
@@ -177,7 +177,8 @@
 state.writing = preview.enabled() ?
 State::writingPreview : State::writingPrime;
 virginWriteClaim.protectAll();
-