Build failed in Jenkins: 3.2-matrix » opensuse-x64 #263

[noc]

See 


Changes:

[Amos Jeffries] Bug 3615: configure check for default max number of FDs is 
broken

[Amos Jeffries] Fix maximum_single_addr_tries upgrade

[Amos Jeffries] Bug 3622: peerClearRRStart scheduling multiple events

[Amos Jeffries] Bug 3626: Forwarding loops on intercepted traffic

Changes to interception handling in 3.2 series (namely the preference
for using ORIGINAL_DST) have increased the chances that misconfigured
network systems involving Squid will hit forwarding loops.

Two instances are currently known:
* passing forward-proxy traffic to a interception port.
* NAT performed on a separate box.

This enacts an old TODO by removing the loop detection bypass for
intercepted traffic and accelerated traffic. Now we always check for
loops regardless of how the request was received.

NOTE: accel mode was only included due to the TODO.
If problems are found there it can be re-instated.

[Amos Jeffries] Make CpuAffinitySet::applied() method non-const.

According to CPU_SET(3) and, apparently, on some systems (e.g.,
OpenSuSE 10.3) CPU_COUNT macro expects a non-const argument.  The
patch fixes build error on these systems.

[Amos Jeffries] Bug 3609: several RADIUS helper improvements

[Automatic source maintenance] SourceFormat Enforcement

--
[...truncated 6935 lines...]
Testing ../../src/FileMap.h ...Ok.
Testing ../../src/ConfigParser.h ...Ok.
Testing ../../src/client_side.h ...Ok.
Testing ../../src/StoreMeta.h ...Ok.
Testing ../../src/SquidMath.h ...Ok.
Testing ../../src/LeakFinder.h ...Ok.
Testing ../../src/DelayIdComposite.h ...Ok.
Testing ../../src/CacheManager.h ...Ok.
Testing ../../src/CpuAffinitySet.h ...Ok.
Testing ../../src/URLScheme.h ...Ok.
Testing ../../src/MemBuf.h ...Ok.
Testing ../../src/DelayId.h ...Ok.
Testing ../../src/BodyPipe.h ...Ok.
Testing ../../src/HttpReply.h ...Ok.
Testing ../../src/StatHist.h ...Ok.
Testing ../../src/HttpHeaderStat.h ...Ok.
Testing ../../src/mem_node.h ...Ok.
Testing ../../src/DelayPool.h ...Ok.
Testing ../../src/StoreMetaObjSize.h ...Ok.
Testing ../../src/helper.h ...Ok.
Testing ../../src/errorpage.h ...Ok.
Testing ../../src/ClientInfo.h ...Ok.
Testing ../../src/HttpStatusLine.h ...Ok.
Testing ../../src/StoreIOBuffer.h ...Ok.
Testing ../../src/LoadableModule.h ...Ok.
Testing ../../src/typedefs.h ...Ok.
Testing ../../src/ipcache.h ...Ok.
Testing ../../src/comm_err_t.h ...Ok.
Testing ../../src/clientStream.h ...Ok.
Testing ../../src/FadingCounter.h ...Ok.
Testing ../../src/client_side_request.h ...Ok.
Testing ../../src/Mem.h ...Ok.
Testing ../../src/HttpHdrCc.h ...Ok.
Testing ../../src/htcp.h ...Ok.
Testing ../../src/StoreSearch.h ...Ok.
Testing ../../src/pconn.h ...Ok.
Testing ../../src/CpuAffinity.h ...Ok.
Testing ../../src/Generic.h ...Ok.
Testing ../../src/ETag.h ...Ok.
Testing ../../src/CpuAffinityMap.h ...Ok.
Testing ../../src/MemObject.h ...Ok.
Testing ../../src/SwapDir.h ...Ok.
Testing ../../src/ClientRequestContext.h ...Ok.
Testing ../../src/HelperChildConfig.h ...Ok.
Testing ../../src/StoreMetaUnpacker.h ...Ok.
Testing ../../src/err_detail_type.h ...Ok.
Testing ../../src/cbdata.h ...Ok.
Testing ../../src/lookup_t.h ...Ok.
Testing ../../src/snmp_core.h ...Ok.
Testing ../../src/URL.h ...Ok.
Testing ../../src/wordlist.h ...Ok.
Testing ../../src/StoreFileSystem.h ...Ok.
Testing ../../src/AccessLogEntry.h ...Ok.
Testing ../../src/ConfigOption.h ...Ok.
Testing ../../src/CommCalls.h ...Ok.
Testing ../../src/PeerSelectState.h ...Ok.
Testing ../../src/EventLoop.h ...Ok.
Testing ../../src/forward.h ...Ok.
Testing ../../src/DelayPools.h ...Ok.
Testing ../../src/ChunkedCodingParser.h ...Ok.
Testing ../../src/Store.h ...Ok.
Testing ../../src/StoreMetaMD5.h ...Ok.
Testing ../../src/enums.h ...Ok.
Testing ../../src/HttpHeaderMask.h ...Ok.
Testing ../../src/StoreIOState.h ...Ok.
Testing ../../src/TimeOrTag.h ...Ok.
Testing ../../src/DelayConfig.h ...Ok.
Testing ../../src/hier_code.h ...Ok.
Testing ../../src/DelayUser.h ...Ok.
Testing ../../src/HttpParser.h ...Ok.
Testing ../../src/MemBlob.h ...Ok.
Testing ../../src/HttpStatusCode.h ...Ok.
Testing ../../src/icp_opcode.h ...Ok.
Testing ../../src/HttpHdrContRange.h ...Ok.
Testing ../../src/SquidDns.h ...Ok.
Testing ../../src/globals.h ...Ok.
Testing ../../src/DelayVector.h ...Ok.
Testing ../../src/http.h ...Ok.
Testing ../../src/HttpHdrScTarget.h ...Ok.
Testing ../../src/SquidTime.h ...Ok.
Testing ../../src/PingData.h ...Ok.
Testing ../../src/Parsing.h ...Ok.
Testing ../../src/HttpBody.h ...Ok.
Testing ../../src/StoreMetaSTD.h ...Ok.
Testing ../../src/event.h ...Ok.
Testing ../../src/DescriptorSet.h ...Ok.
Testing ../../src/comm.h ...Ok.
Testing ../../src/DelayBucket.h ...Ok.
Testing ../../src/LoadableModules.h ...Ok.
Testing ../../src/stmem.h ...Ok.
Testing ../../src/CommRead.h ...Ok.
Testing ../../src/CompositePoolNode.h ...Ok.
Testing ../../src/Server.h ...Ok.
Testing ../../src/DelayTagged.h ..

Build failed in Jenkins: 3.2-matrix » obsd-51-x86 #263

[noc]

See 

--
Started by upstream project "3.2-matrix" build number 263
Building remotely on obsd-51-x86 in workspace 

java.io.IOException: Failed to mkdirs: 

at hudson.FilePath.mkdirs(FilePath.java:901)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1254)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:589)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:88)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:494)
at hudson.model.Run.execute(Run.java:1502)
at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
at hudson.model.ResourceController.execute(ResourceController.java:88)
at hudson.model.Executor.run(Executor.java:236)
Retrying after 10 seconds
java.io.IOException: Failed to mkdirs: 

at hudson.FilePath.mkdirs(FilePath.java:901)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1254)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:589)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:88)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:494)
at hudson.model.Run.execute(Run.java:1502)
at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
at hudson.model.ResourceController.execute(ResourceController.java:88)
at hudson.model.Executor.run(Executor.java:236)
Retrying after 10 seconds
java.io.IOException: Failed to mkdirs: 

at hudson.FilePath.mkdirs(FilePath.java:901)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1254)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:589)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:88)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:494)
at hudson.model.Run.execute(Run.java:1502)
at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
at hudson.model.ResourceController.execute(ResourceController.java:88)
at hudson.model.Executor.run(Executor.java:236)

Re: [RFC] CONNECT and use_ssl cache_peer

[Amos Jeffries]

On 10.09.2012 13:08, Henrik Nordström wrote:

sön 2012-09-09 klockan 21:34 +1200 skrev Amos Jeffries:

Henrik and I seem to disagree on this being a good idea being 
plugged
into BodyPipe. But as I see it BodyPipe is where Upgrade, WebSockets 
and
SSL-Bump should be happening in a node which internally "receives" 
the
tunnel connection and acts like a ConnStateData reading out of the 
tunnel.


I am open for discussion on how it should fit, but initial reaction 
is
that there may be too much http semantics in the message model there 
to

fit well.


I'm imagining a setup of:

 client TCP -> ConnStateData(BodyPipeProducer) -> Bump(BodyPipeReader, 
'fake' ConnStateData(BodyPipeProducer) ) -> BodyPipe -> server-side


so that:
 * the bumped CONNECT tunnel can be the source of a whole pipeline of 
requests (HTTPS, etc)
 * the original ConnStateData->Bump section of code retains all the 
CONNECT semantics down to and including terminate 'server' connection 
(aka Bump Node destructs) on end of CONNECT independent of what Bump -> 
server-side does with the server-side connection.
 * when receiving a CONNECT we detect the right type of streams reader 
to handle the data and allocate it:
   ** the Bump node could be Upgrade:TLS, Upgrade:WebSockets, 
Upgrade:HTTP/2, SSL-bump, pass-thru tunnel or some future protocol




When going DIRECT we have to assume the CONNECT is infinite length 
and

handle as per now with closures.


You always have to assume that unless there is some other indication.



That other indication would be Content-Length or chunked encoding yes? 
which currently are ignored, with some annoying results.



But ... I see a potential gap in the RFC texts that when 
communicating
between two proxies we can do one of several methods (Expect-100 or 
peer

OPTIONS) to negotiate chunking support for any data the CONNECT has
following it.


Not sure there is a gap, but I see your point. The data of a CONNECT 
is

not the message body, not more than the data following an Upgrade
handshake is.


The gap is that chunking can be (and is!) implemented on the opaque 
data stream after POST+Upgrade, GET+Upgrade, FOO+Upgrade - yet is 
undefined and not implemented for CONNECT+Upgrade or plain CONNECT. The 
prose describes CONNECT as having no body which implies there is nothing 
to chunk, but in practice there is usually opaque data there to be 
relayed same as for POST.




There is not much negotiation taking place in CONNECT. Any extensions
has to be enabled by configuration. The 1.1+ protocol cache which can 
be

used to negotiate chunked encoding in requests for other methods is
useless for CONNECT as we cannot expect HTTP/1.1 servers to accept
CONNECT with transfer-encoding.


It would have to be a separate flag/negotiation from any other method 
yes. But I think the server 'cache' can still flag it for use across 
subsequent CONNECT requests.





Thus keeping the connection between the peers (only)
re-usable after the CONNECT is finished. So long as the 
transformation
(chunked encoding) is stripped away at the recipient proxy this does 
not

introduce problems to the transported end-to-end data stream.


Yes...

Regards
Henrik


Amos

Re: [RFC] CONNECT and use_ssl cache_peer

[Henrik Nordström]

sön 2012-09-09 klockan 21:34 +1200 skrev Amos Jeffries:

> Henrik and I seem to disagree on this being a good idea being plugged 
> into BodyPipe. But as I see it BodyPipe is where Upgrade, WebSockets and 
> SSL-Bump should be happening in a node which internally "receives" the 
> tunnel connection and acts like a ConnStateData reading out of the tunnel.

I am open for discussion on how it should fit, but initial reaction is
that there may be too much http semantics in the message model there to
fit well.

> When going DIRECT we have to assume the CONNECT is infinite length and 
> handle as per now with closures.

You always have to assume that unless there is some other indication.

> But ... I see a potential gap in the RFC texts that when communicating 
> between two proxies we can do one of several methods (Expect-100 or peer 
> OPTIONS) to negotiate chunking support for any data the CONNECT has 
> following it.

Not sure there is a gap, but I see your point. The data of a CONNECT is
not the message body, not more than the data following an Upgrade
handshake is.

There is not much negotiation taking place in CONNECT. Any extensions
has to be enabled by configuration. The 1.1+ protocol cache which can be
used to negotiate chunked encoding in requests for other methods is
useless for CONNECT as we cannot expect HTTP/1.1 servers to accept
CONNECT with transfer-encoding.

> Thus keeping the connection between the peers (only) 
> re-usable after the CONNECT is finished. So long as the transformation 
> (chunked encoding) is stripped away at the recipient proxy this does not 
> introduce problems to the transported end-to-end data stream.

Yes...

Regards
Henrik

Re: Store_url_rewrite for squid 3+

[Amos Jeffries]

On 10.09.2012 04:56, Eliezer Croitoru wrote:

On 09/06/2012 03:58 AM, Amos Jeffries wrote:


I don't think there is anything which needs new cache_cf.cc code. 
The parsing side if things is identical for url_rewrite_*. The 
different defaults and locations are all coded in cf.data.pre ... yes 
indeed but the actual effect comes from the code in  cache_cf.cc

an example is:
 if (Config.Program.redirect) {
if (Config.redirectChildren.n_max < 1) {
Config.redirectChildren.n_max = 0;
wordlistDestroy(&Config.Program.redirect);
}
}it's specific for the redirect program.

I have tried to use the helper but it seems like without the
cache_cf.cc code it wil assume the number of default helpers is 0/20
and will not start any.
which is werid...



Ah. Okay I had forgotten that hack. Yes it is needed for store-url as 
well.


It is for the case where a program name is configured but there are no 
children to be started. Or on reconfigure where a program used to be 
configured but is now removed from the config.


0/20 with none started straight away is the default. One will be 
started on first request through the proxy that needs the helper.


Amos

Re: [PATCH]proposal for a fake_strore_url helper wrapping. +fixes

[Amos Jeffries]

On 10.09.2012 01:12, Eliezer Croitoru wrote:

On 9/9/2012 2:54 PM, Amos Jeffries wrote:


  ++  It is questionable whether the store-URL should also be below 
the

no_cache_done bits as well. Since requests with"cache deny" are not
going to be loaded from cache, but maybe they will be stored later 
by
future Squid (which will need store-URL changes then) - in current 
squid

they are useless passing to the storeurl helper.

When we will be there then...
Are there any visible plans?


Not as yet. It just shows up as a HTTP/1.1 non-compliance TODO entry 
about making "Cache-Control:no-cache" from the client prevent cache read 
but allow cache write instead of having it discard the response.



Amos

Re: BZR repository upgrade re-proposal.

[Kinkie]

On Sun, Sep 9, 2012 at 9:36 PM, Henrik Nordström
 wrote:
> sön 2012-09-09 klockan 19:15 +0200 skrev Kinkie:
>
>> It seems that the schedule was missed.
>
> Yes. Been ill since friday. Combination of sleep deprivation and a cold
> taking it's poll.

Sorry to hear that. Get well soon!

>> When is the next chance?
>
> Haven't seen any indications of any remaining reasons to not being able
> to switch.
>
> Wednesday maybe? Not feeling too alert tonight and should probably not
> do anything needing privileges, and monday & tuesday is a bit choked
> already.

Sounds like a good plan.
Thanks!


-- 
/kinkie

Re: BZR repository upgrade re-proposal.

[Henrik Nordström]

sön 2012-09-09 klockan 19:15 +0200 skrev Kinkie:

> It seems that the schedule was missed.

Yes. Been ill since friday. Combination of sleep deprivation and a cold
taking it's poll.


> When is the next chance?

Haven't seen any indications of any remaining reasons to not being able
to switch.

Wednesday maybe? Not feeling too alert tonight and should probably not
do anything needing privileges, and monday & tuesday is a bit choked
already.

Regards
Henrik

Re: [PATCH]I will have more look at it tommorrow

[Eliezer Croitoru]

On 09/09/2012 07:06 PM, Eliezer Croitoru wrote:

On 09/09/2012 06:31 PM, Eliezer Croitoru wrote:


Fix those and I think this is a good part-1 patch that can be 
applied to

trunk.

Amos




found missing declaration in the flags struct.
a fixed patch included.

and a tiny tiny really weird typo.
Tested a bit and it compiled but when i starts url_rewrite_helper seems 
like it's because of something.

so I will look at it tomorrow to find out what exactly i missed.


Eliezer

Re: BZR repository upgrade re-proposal.

[Kinkie]

  I'm glad that a consensus is forming.
 The next question would then be who will perform the upgrade, and when
 - assuming noone speaks up against it.
>>>
>>>
>>> This friday?
>>
>>
>> +1 :)
>
>
>
> +1.

It seems that the schedule was missed.
When is the next chance?

Thanks,

-- 
/kinkie

Re: Store_url_rewrite for squid 3+

[Eliezer Croitoru]

On 09/06/2012 03:58 AM, Amos Jeffries wrote:


I don't think there is anything which needs new cache_cf.cc code. The 
parsing side if things is identical for url_rewrite_*. The different 
defaults and locations are all coded in cf.data.pre ... 

yes indeed but the actual effect comes from the code in  cache_cf.cc
an example is:
 if (Config.Program.redirect) {
if (Config.redirectChildren.n_max < 1) {
Config.redirectChildren.n_max = 0;
wordlistDestroy(&Config.Program.redirect);
}
}it's specific for the redirect program.

I have tried to use the helper but it seems like without the cache_cf.cc 
code it wil assume the number of default helpers is 0/20 and will not 
start any.

which is werid...


Eliezer

Re: [PATCH]fix to the fix - was a typo.

[Eliezer Croitoru]

On 09/09/2012 06:31 PM, Eliezer Croitoru wrote:


Fix those and I think this is a good part-1 patch that can be 
applied to

trunk.

Amos




found missing declaration in the flags struct.
a fixed patch included.

and a tiny tiny really weird typo.
=== modified file 'src/ClientRequestContext.h'
--- src/ClientRequestContext.h	2012-08-14 11:53:07 +
+++ src/ClientRequestContext.h	2012-09-09 12:52:34 +
@@ -33,6 +33,8 @@
 void clientAccessCheckDone(const allow_t &answer);
 void clientRedirectStart();
 void clientRedirectDone(char *result);
+void clientStoreurlStart();
+void clientStoreurlDone(char *result);
 void checkNoCache();
 void checkNoCacheDone(const allow_t &answer);
 #if USE_ADAPTATION
@@ -53,7 +55,9 @@
 ClientHttpRequest *http;
 ACLChecklist *acl_checklist;/* need ptr back so we can unreg if needed */
 int redirect_state;
+int storeurl_state;
 
+bool storeurl_done;
 bool host_header_verify_done;
 bool http_access_done;
 bool adapted_http_access_done;

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2012-09-06 13:12:26 +
+++ src/client_side.cc	2012-09-09 12:54:28 +
@@ -714,6 +714,7 @@
 ClientHttpRequest::freeResources()
 {
 safe_free(uri);
+safe_free(store_uri);
 safe_free(log_uri);
 safe_free(redirect.location);
 range_iter.boundary.clean();

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2012-08-28 19:12:13 +
+++ src/client_side_request.cc	2012-09-09 15:56:52 +
@@ -131,6 +131,7 @@
 static int clientHierarchical(ClientHttpRequest * http);
 static void clientInterpretRequestHeaders(ClientHttpRequest * http);
 static RH clientRedirectDoneWrapper;
+static RH clientStoreurlDoneWrapper;
 static void checkNoCacheDoneWrapper(allow_t, void *);
 extern "C" CSR clientGetMoreData;
 extern "C" CSS clientReplyStatus;
@@ -151,10 +152,11 @@
 debugs(85,3, HERE << this << " ClientRequestContext destructed");
 }
 
-ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
+ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), storeurl_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
 {
 http_access_done = false;
 redirect_done = false;
+storeurl_done = false;
 no_cache_done = false;
 interpreted_req_hdrs = false;
 #if USE_SSL
@@ -916,6 +918,32 @@
 redirectStart(http, clientRedirectDoneWrapper, this);
 }
 
+static void
+clientStoreurlAccessCheckDone(allow_t answer, void *data)
+{
+ClientRequestContext *context = (ClientRequestContext *)data;
+ClientHttpRequest *http = context->http;
+context->acl_checklist = NULL;
+
+if (answer == ACCESS_ALLOWED)
+storeurlStart(http, clientStoreurlDoneWrapper, context);
+else
+context->clientStoreurlDone(NULL);
+}
+
+void
+ClientRequestContext::clientStoreurlStart()
+{
+debugs(33, 5, HERE << "' ClientRequestContext::clientStoreurlStart " << http->uri << "'");
+
+if (Config.accessList.storeurl) {
+acl_checklist = clientAclChecklistCreate(Config.accessList.storeurl, http);
+acl_checklist->nonBlockingCheck(clientStoreurlAccessCheckDone, this);
+} else {
+	clientStoreurlDone(NULL);
+}
+}
+
 static int
 clientHierarchical(ClientHttpRequest * http)
 {
@@ -1262,6 +1290,37 @@
 http->doCallouts();
 }
 
+void
+clientStoreurlDoneWrapper(void *data, char *result)
+{
+ClientRequestContext *calloutContext = (ClientRequestContext *)data;
+
+if (!calloutContext->httpStateIsValid())
+return;
+
+calloutContext->clientStoreurlDone(result);
+}
+
+void
+ClientRequestContext::clientStoreurlDone(char *result)
+{
+debugs(85, 5, "clientStoreurlDone: '" << http->uri << "' result=" << (result ? result : "NULL"));
+assert(storeurl_state == REDIRECT_PENDING);
+storeurl_state = REDIRECT_DONE;
+
+/*
+ * the next step is to implement here the actuall store_url_rewrite proccess of pushing
+ * the store_url into thr http request.
+ */
+if (result) {
+	debugs(85, DBG_CRITICAL, "A result for storu_url accepted: " << result);
+	} else {
+		debugs(85, DBG_CRITICAL, "A result for storu_url was not accepted: " << result);
+}
+
+http->doCallouts();
+}
+
 /** Test cache allow/deny configuration
  *  Sets flags.cachable=1 if caching is not denied.
  */
@@ -1689,6 +1748,18 @@
 if (ih != NULL)
 ih->logType = logType;
 #endif
+
+if (!calloutContext->storeurl_done) {
+		calloutContext->storeurl_done = true;
+		assert(calloutContext->storeurl_state == REDIRECT_NONE);
+
+		if (Config.Program.storeurl) {
+			debugs(83, 3, HERE << "Doing calloutContext->clientSotreurlStart()");
+			calloutContext->storeurl_state = REDIRECT_PENDING;
+			calloutContext->cl

Re: [PATCH]fix to the fix

[Eliezer Croitoru]

Fix those and I think this is a good part-1 patch that can be applied to
trunk.

Amos




found missing declaration in the flags struct.
a fixed patch included.
=== modified file 'src/ClientRequestContext.h'
--- src/ClientRequestContext.h	2012-08-14 11:53:07 +
+++ src/ClientRequestContext.h	2012-09-09 12:52:34 +
@@ -33,6 +33,8 @@
 void clientAccessCheckDone(const allow_t &answer);
 void clientRedirectStart();
 void clientRedirectDone(char *result);
+void clientStoreurlStart();
+void clientStoreurlDone(char *result);
 void checkNoCache();
 void checkNoCacheDone(const allow_t &answer);
 #if USE_ADAPTATION
@@ -53,7 +55,9 @@
 ClientHttpRequest *http;
 ACLChecklist *acl_checklist;/* need ptr back so we can unreg if needed */
 int redirect_state;
+int storeurl_state;
 
+bool storeurl_done;
 bool host_header_verify_done;
 bool http_access_done;
 bool adapted_http_access_done;

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2012-09-06 13:12:26 +
+++ src/client_side.cc	2012-09-09 12:54:28 +
@@ -714,6 +714,7 @@
 ClientHttpRequest::freeResources()
 {
 safe_free(uri);
+safe_free(store_uri);
 safe_free(log_uri);
 safe_free(redirect.location);
 range_iter.boundary.clean();

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2012-08-28 19:12:13 +
+++ src/client_side_request.cc	2012-09-09 13:01:23 +
@@ -131,6 +131,7 @@
 static int clientHierarchical(ClientHttpRequest * http);
 static void clientInterpretRequestHeaders(ClientHttpRequest * http);
 static RH clientRedirectDoneWrapper;
+static RH clientStoreurlDoneWrapper;
 static void checkNoCacheDoneWrapper(allow_t, void *);
 extern "C" CSR clientGetMoreData;
 extern "C" CSS clientReplyStatus;
@@ -151,10 +152,11 @@
 debugs(85,3, HERE << this << " ClientRequestContext destructed");
 }
 
-ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
+ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), storeurl_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
 {
 http_access_done = false;
 redirect_done = false;
+storeurl_done = false;
 no_cache_done = false;
 interpreted_req_hdrs = false;
 #if USE_SSL
@@ -916,6 +918,31 @@
 redirectStart(http, clientRedirectDoneWrapper, this);
 }
 
+static void
+clientStoreurlAccessCheckDone(allow_t answer, void *data)
+{
+ClientRequestContext *context = (ClientRequestContext *)data;
+ClientHttpRequest *http = context->http;
+context->acl_checklist = NULL;
+
+if (answer == ACCESS_ALLOWED)
+storeurlStart(http, clientStoreurlDoneWrapper, context);
+else
+context->clientStoreurlDone(NULL);
+}
+
+void
+ClientRequestContext::clientStoreurlStart()
+{
+debugs(33, 5, HERE << "' ClientRequestContext::clientStoreurlStart " << http->uri << "'");
+
+if (Config.accessList.storeurl) {
+acl_checklist = clientAclChecklistCreate(Config.accessList.storeurl, http);
+acl_checklist->nonBlockingCheck(clientStoreurlAccessCheckDone, this);
+} else
+	clientStoreurlDone(NULL)
+}
+
 static int
 clientHierarchical(ClientHttpRequest * http)
 {
@@ -1262,6 +1289,37 @@
 http->doCallouts();
 }
 
+void
+clientStoreurlDoneWrapper(void *data, char *result)
+{
+ClientRequestContext *calloutContext = (ClientRequestContext *)data;
+
+if (!calloutContext->httpStateIsValid())
+return;
+
+calloutContext->clientStoreurlDone(result);
+}
+
+void
+ClientRequestContext::clientStoreurlDone(char *result)
+{
+debugs(85, 5, "clientStoreurlDone: '" << http->uri << "' result=" << (result ? result : "NULL"));
+assert(storeurl_state == REDIRECT_PENDING);
+storeurl_state = REDIRECT_DONE;
+
+/*
+ * the next step is to implement here the actuall store_url_rewrite proccess of pushing
+ * the store_url into thr http request.
+ */
+if (result) {
+	debugs(85, DBG_CRITICAL, "A result for storu_url accepted: " << result);
+	} else {
+		debugs(85, DBG_CRITICAL, "A result for storu_url was not accepted: " << result);
+}
+
+http->doCallouts();
+}
+
 /** Test cache allow/deny configuration
  *  Sets flags.cachable=1 if caching is not denied.
  */
@@ -1689,6 +1747,18 @@
 if (ih != NULL)
 ih->logType = logType;
 #endif
+
+if (!calloutContext->storeurl_done) {
+		calloutContext->storeurl_done = true;
+		assert(calloutContext->storeurl_state == REDIRECT_NONE);
+
+		if (Config.Program.storeurl) {
+			debugs(83, 3, HERE << "Doing calloutContext->clientSotreurlStart()");
+			calloutContext->storeurl_state = REDIRECT_PENDING;
+			calloutContext->clientStoreurlStart();
+			return;
+		}
+	}
 }
 
 #if !_USE_INLINE_

=== modified file 'src/client

Re: [PATCH]proposal for a fake_strore_url helper wrapping. +fixes

[Eliezer Croitoru]

On 9/9/2012 2:54 PM, Amos Jeffries wrote:

On 9/09/2012 10:08 p.m., Eliezer Croitoru wrote:

I took the url_rewrite helper and used what ever seems needed to
create a url_store_rewrite helper.
I also added the needed variables for the next step and freeing them
where and if needed.
this is a more of a basic sketch.

Eliezer



You dont need all the "added for storeurl", "changed X" comments, they
just bloat the code and patch.

OK,these was more for me.
Recommendations on how to manage myself in such huge code are more then 
welcome.(using eclipse)


Please don't add multiple empty lines in a row. This is happening in
several places.

will be fixed.



src/client_side_request.cc:

* in ClientRequestContext::clientStoreurlStart the else condition is
what gets done if there is no access list configured. This should be
defaulting to *not* changing the store-URL. call
clientStoreurlDone(NULL) instead of storeurlStart()

Done.



* in doCallouts() please move the store-url bits to after adaptation and
URL-redirect bits. Those checks may result in a non-storable response
being generated by Squid straight to the client.

I wasn't sure about it, Thanks.


  ++  It is questionable whether the store-URL should also be below the
no_cache_done bits as well. Since requests with"cache deny" are not
going to be loaded from cache, but maybe they will be stored later by
future Squid (which will need store-URL changes then) - in current squid
they are useless passing to the storeurl helper.

When we will be there then...
Are there any visible plans?



src/client_side_request.h:
* please name the new variable "store_uri" in line with the other
surrounding variable naming style.

Done



src/redirect.cc:
* why is storeurlHandleReply() being added? all it does is receive the
reply and call a callback handler. It is that handler which is the only
part store-url specific and set by storeurlStart. If it was just the
debugs, they should be changed to  debugs(61, 5, HERE << "{...

Thanks. applied


* in redirectInit() you are using new on the redirectors variable when
it should be the storeurl_rewriters variable.

oops. ~0-0~
Fixed



To answer your query comment... CBDATA_INIT_TYPE does memory allocation
and new/delete method definition setup for the redirectStateData
objects. Since they are shared by both systems you can ignore that part
of redirectInit().




Although the static variable should be moved down
next to the if(init) and changed to a bool type - that is irrelevant to
the storeurl stuff though.

Well i'm already here so..


* in storeUrlStart() the INTERNAL_SERVER_ERROR log message is still
saying "redirector"

fixed



Fix those and I think this is a good part-1 patch that can be applied to
trunk.

Amos



--
Eliezer Croitoru
https://www1.ngtech.co.il
IT consulting for Nonprofit organizations
eliezer  ngtech.co.il
=== modified file 'src/ClientRequestContext.h'
--- src/ClientRequestContext.h	2012-08-14 11:53:07 +
+++ src/ClientRequestContext.h	2012-09-09 12:52:34 +
@@ -33,6 +33,8 @@
 void clientAccessCheckDone(const allow_t &answer);
 void clientRedirectStart();
 void clientRedirectDone(char *result);
+void clientStoreurlStart();
+void clientStoreurlDone(char *result);
 void checkNoCache();
 void checkNoCacheDone(const allow_t &answer);
 #if USE_ADAPTATION
@@ -53,7 +55,9 @@
 ClientHttpRequest *http;
 ACLChecklist *acl_checklist;/* need ptr back so we can unreg if needed */
 int redirect_state;
+int storeurl_state;
 
+bool storeurl_done;
 bool host_header_verify_done;
 bool http_access_done;
 bool adapted_http_access_done;

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2012-09-06 13:12:26 +
+++ src/client_side.cc	2012-09-09 12:54:28 +
@@ -714,6 +714,7 @@
 ClientHttpRequest::freeResources()
 {
 safe_free(uri);
+safe_free(store_uri);
 safe_free(log_uri);
 safe_free(redirect.location);
 range_iter.boundary.clean();

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2012-08-28 19:12:13 +
+++ src/client_side_request.cc	2012-09-09 13:01:23 +
@@ -131,6 +131,7 @@
 static int clientHierarchical(ClientHttpRequest * http);
 static void clientInterpretRequestHeaders(ClientHttpRequest * http);
 static RH clientRedirectDoneWrapper;
+static RH clientStoreurlDoneWrapper;
 static void checkNoCacheDoneWrapper(allow_t, void *);
 extern "C" CSR clientGetMoreData;
 extern "C" CSS clientReplyStatus;
@@ -151,10 +152,11 @@
 debugs(85,3, HERE << this << " ClientRequestContext destructed");
 }
 
-ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
+ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), storeurl_state (REDIRECT_NONE), error(NULL), 

Re: [PATCH]proposal for a fake_strore_url helper wrapping.

[Amos Jeffries]

On 9/09/2012 10:08 p.m., Eliezer Croitoru wrote:
I took the url_rewrite helper and used what ever seems needed to 
create a url_store_rewrite helper.
I also added the needed variables for the next step and freeing them 
where and if needed.

this is a more of a basic sketch.

Eliezer



You dont need all the "added for storeurl", "changed X" comments, they 
just bloat the code and patch.


Please don't add multiple empty lines in a row. This is happening in 
several places.


src/client_side_request.cc:

* in ClientRequestContext::clientStoreurlStart the else condition is 
what gets done if there is no access list configured. This should be 
defaulting to *not* changing the store-URL. call 
clientStoreurlDone(NULL) instead of storeurlStart()


* in doCallouts() please move the store-url bits to after adaptation and 
URL-redirect bits. Those checks may result in a non-storable response 
being generated by Squid straight to the client.
 ++  It is questionable whether the store-URL should also be below the 
no_cache_done bits as well. Since requests with"cache deny" are not 
going to be loaded from cache, but maybe they will be stored later by 
future Squid (which will need store-URL changes then) - in current squid 
they are useless passing to the storeurl helper.


src/client_side_request.h:
* please name the new variable "store_uri" in line with the other 
surrounding variable naming style.


src/redirect.cc:
* why is storeurlHandleReply() being added? all it does is receive the 
reply and call a callback handler. It is that handler which is the only 
part store-url specific and set by storeurlStart. If it was just the 
debugs, they should be changed to  debugs(61, 5, HERE << "{...


* in redirectInit() you are using new on the redirectors variable when 
it should be the storeurl_rewriters variable.


To answer your query comment... CBDATA_INIT_TYPE does memory allocation 
and new/delete method definition setup for the redirectStateData 
objects. Since they are shared by both systems you can ignore that part 
of redirectInit(). Although the static variable should be moved down 
next to the if(init) and changed to a bool type - that is irrelevant to 
the storeurl stuff though.


* in storeUrlStart() the INTERNAL_SERVER_ERROR log message is still 
saying "redirector"



Fix those and I think this is a good part-1 patch that can be applied to 
trunk.


Amos

Re: ICAP connections under heavy loads

[Alexander Komyagin]

On Fri, 2012-09-07 at 09:17 -0600, Alex Rousskov wrote:
> On 09/07/2012 08:33 AM, Alexander Komyagin wrote:
> > On Fri, 2012-09-07 at 08:15 -0600, Alex Rousskov wrote:
> >> On 09/07/2012 02:32 AM, Alexander Komyagin wrote:
> >>> However, as I stated earlier, the comm.cc problem (actually semantics
> >>> problem) persists. I think it should be documented that second and
> >>> subsequent calls to comm_connect_addr() do not guarantee connection
> >>> establishment unless there was a correct select() notification.
> 
> >> Agreed: We should document what comm_connect_addr() does. However, does
> >> it provide any guarantees even _after_ select() notification? According
> >> to Stevens, the current code may still report that there is no problem
> >> when in fact there is one (and we will detect it later during I/O), right?
> 
> > After select() notification there are two scenarios possible:
> > 1) connection was successfully established (we got EPOLLOUT there). Then
> > getsockopt() would correctly report success;
> > 2) there was a problem (we got EPOLLHUP or EPOLLERR). In this case
> > getsockopt() shall report an error (i.e. appropriate errno).
> > 
> > So I think we can rely on comm_connect_addr(), but only _after_ the
> > notification.
> 
> 
> This is not how I understand the comm_connect_addr() call context.
> comm_connect_addr() is called by ConnOpener in at least two cases:
> 
> 1. To initiate the connection, after comm_openex in ConnOpener::start().
> 
> 2. After select(2) or equivalent said that the socket is ready for writing.
> 
> 
> Let's focus on the second context. We know the socket is writeable. This
> can mean that either (2a) the connection was established (or at least
> ready for writing) OR (2b) that there was a problem establishing the
> connection.
> 
> Can a getsockopt() call in case 2b guarantee problem detection? If my
> reading of Stevens explanation is correct, it cannot (because Stevens
> suggests three ways to get that guarantee instead of calling getsockopt,
> implying that getsockopt is not reliable in this context).

In my opinion, these (getpeername(), etc.) are just alternatives to
getsockopt() in the context where "writability is not the only way
success is returned". Example of such a context is checking connection
establishment status without the select() call.

As soon as we plan to issue second comm_connect_addr() call _after_ the
select() notification, getsockopt() is fine.

> 
> If I am right, the new documentation should reflect this uncertainty.
> However, as discussed before, the higher level code will still work even
> if we do not notice the error right away (we will get an error when we
> try to write or read).
> 
> 
> HTH,
> 
> Alex.
> 

-- 
Best wishes,
Alexander Komyagin

[PATCH]proposal for a fake_strore_url helper wrapping.

[Eliezer Croitoru]

I took the url_rewrite helper and used what ever seems needed to create 
a url_store_rewrite helper.
I also added the needed variables for the next step and freeing them 
where and if needed.

this is a more of a basic sketch.

Eliezer

=== modified file 'src/ClientRequestContext.h'
--- src/ClientRequestContext.h	2012-08-14 11:53:07 +
+++ src/ClientRequestContext.h	2012-09-08 14:32:13 +
@@ -33,6 +33,10 @@
 void clientAccessCheckDone(const allow_t &answer);
 void clientRedirectStart();
 void clientRedirectDone(char *result);
+//adding for storeurl
+void clientStoreurlStart();
+void clientStoreurlDone(char *result);
+//end add for storeurl
 void checkNoCache();
 void checkNoCacheDone(const allow_t &answer);
 #if USE_ADAPTATION
@@ -53,7 +57,11 @@
 ClientHttpRequest *http;
 ACLChecklist *acl_checklist;/* need ptr back so we can unreg if needed */
 int redirect_state;
+//added for storeurl
+int storeurl_state;
 
+bool storeurl_done;
+//end add for storeurl
 bool host_header_verify_done;
 bool http_access_done;
 bool adapted_http_access_done;

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2012-09-06 13:12:26 +
+++ src/client_side.cc	2012-09-08 14:44:38 +
@@ -714,6 +714,7 @@
 ClientHttpRequest::freeResources()
 {
 safe_free(uri);
+safe_free(storeurl); //adding for store_url
 safe_free(log_uri);
 safe_free(redirect.location);
 range_iter.boundary.clean();

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2012-08-28 19:12:13 +
+++ src/client_side_request.cc	2012-09-09 09:59:52 +
@@ -131,6 +131,7 @@
 static int clientHierarchical(ClientHttpRequest * http);
 static void clientInterpretRequestHeaders(ClientHttpRequest * http);
 static RH clientRedirectDoneWrapper;
+static RH clientStoreurlDoneWrapper; //adding for storeurl
 static void checkNoCacheDoneWrapper(allow_t, void *);
 extern "C" CSR clientGetMoreData;
 extern "C" CSS clientReplyStatus;
@@ -151,10 +152,11 @@
 debugs(85,3, HERE << this << " ClientRequestContext destructed");
 }
 
-ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
+ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), storeurl_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
 {
 http_access_done = false;
 redirect_done = false;
+storeurl_done = false; //added for storeurl and also at the method line
 no_cache_done = false;
 interpreted_req_hdrs = false;
 #if USE_SSL
@@ -915,6 +917,32 @@
 } else
 redirectStart(http, clientRedirectDoneWrapper, this);
 }
+//adding for storeurl
+static void
+clientStoreurlAccessCheckDone(allow_t answer, void *data)
+{
+ClientRequestContext *context = (ClientRequestContext *)data;
+ClientHttpRequest *http = context->http;
+context->acl_checklist = NULL;
+
+if (answer == ACCESS_ALLOWED)
+storeurlStart(http, clientStoreurlDoneWrapper, context);
+else
+context->clientStoreurlDone(NULL);
+}
+
+void
+ClientRequestContext::clientStoreurlStart()
+{
+debugs(33, 5, HERE << "' ClientRequestContext::clientStoreurlStart " << http->uri << "'");
+
+if (Config.accessList.storeurl) {
+acl_checklist = clientAclChecklistCreate(Config.accessList.storeurl, http);
+acl_checklist->nonBlockingCheck(clientStoreurlAccessCheckDone, this);
+} else
+storeurlStart(http, clientStoreurlDoneWrapper, this);
+}
+//end add for storeurl add
 
 static int
 clientHierarchical(ClientHttpRequest * http)
@@ -1261,6 +1289,40 @@
 
 http->doCallouts();
 }
+//adding for storeurl
+
+void
+clientStoreurlDoneWrapper(void *data, char *result)
+{
+ClientRequestContext *calloutContext = (ClientRequestContext *)data;
+
+if (!calloutContext->httpStateIsValid())
+return;
+
+calloutContext->clientStoreurlDone(result);
+}
+
+void
+ClientRequestContext::clientStoreurlDone(char *result)
+{
+debugs(85, 5, "clientStoreurlDone: '" << http->uri << "' result=" << (result ? result : "NULL"));
+assert(storeurl_state == REDIRECT_PENDING);
+storeurl_state = REDIRECT_DONE;
+
+/*
+ * the next step is to implement here the actuall store_url_rewrite proccess of pushing
+ * the store_url into thr http request.
+ */
+if (result) {
+	debugs(85, DBG_CRITICAL, "A result for storu_url accepted: " << result);
+	} else {
+		debugs(85, DBG_CRITICAL, "A result for storu_url was not accepted: " << result);
+}
+
+
+http->doCallouts();
+}
+//end add for storeurl
 
 /** Test cache allow/deny configuration
  *  Sets flags.cachable=1 if caching is not denied.
@@ -1584,6 +1646,19 @@
 }
 }
 
+if (!calloutContext->storeurl_done) {
+ 

Re: [RFC] CONNECT and use_ssl cache_peer

[Amos Jeffries]

On 9/09/2012 3:54 a.m., Alex Rousskov wrote:

On 09/07/2012 10:38 PM, Amos Jeffries wrote:

On 8/09/2012 3:54 p.m., Alex Rousskov wrote:

Hello,

  I came upon a Squid proxy configured with "cache_peer ssl" (i.e.,
peer->use_ssl is true). The proxy forwards GETs to the peer using an SSL
connection because forward.cc code knows that "SSL peers" need an SSL
connection and establishes such a connection. This works well.

However, when the proxy receives a CONNECT request, it diverts the
request to tunnel.cc. Tunnel.cc is aware of peer selection logic but
lacks the code that establishes an SSL connection to SSL peers. Squid
establishes a regular TCP connection to the SSL peer. The SSL peer
receives a plain CONNECT request from Squid instead of the expected SSL
handshake and closes the connection with an error.


Here are my top three choices for the fix:

1) Teach tunnel.cc code to establish SSL connections to SSL peers, like
forward.cc does. This design is straightforward, but duplicates
significant (and sometimes rather complex) portions of forward.cc code.
For example, FwdState::initiateSSL() and FwdState::negotiateSSL() would
need to be duplicated (with the exception of SslBump code).


2) Remove peer selection (and SSL peer connection) code from tunnel.cc.
Pass CONNECT requests to forward.cc and allow forward.cc to call
tunnelStart() after selecting (and connecting to) the peer as usual.
This probably requires rewriting tunnel.cc to work with
clientStreams/storeEntries because forward.cc kind of relies on that
API. While I would like to see that kind of design eventually, I do not
want to rewrite so much just to fix this bug. And these more complex
APIs are likely to also make tunneling less efficient.


3) Extract [SSL] peer connection establishment code from tunnel.cc and
forward.cc into a new class. Allow tunnel.cc and forward.cc code to use
that new class to establish the SSL connection as needed, returning
control back to the caller (either tunnel.cc or forward.cc). This avoids
code duplication but SslBump and other code specific to forward.cc will
complicate a clean extraction of the forward.cc code into a stand-alone
peer-connection-establishing class.


My current plan is to try #3 to avoid code duplication but if it becomes
too complex, go for #1 and duplicate what is necessary. What do you
think? Any better ideas?

My vote is for #2 if you can do it. That needs to be done long-term
anyways and will result in less time trying to upkeep separate pathways
in tunnel.cc and forward.cc. The tunnel.cc pathway is already way behind
on features like 1xx status, chunking, keep-alive and pipelined
single-packet requests.

Hi Amos,

 Can you clarify how tunneling is related to chunking, keep-alive and
pipelined single-packet requests? It seems to me that all those HTTP
features are not compatible with CONNECT tunnels because once the tunnel
is established, the connection stops having any HTTP semantics. Are you
thinking about error responses to CONNECT requests? A successful CONNECT
response cannot be chunked (no body), kept alive (no way to determine
the end of the tunnel other than by closing the connection), or handle
more requests (same reason).

As for 1xx messages and error responses, I agree that we should handle
them better in response to Squid's CONNECT request, but we do not handle
them well now, and option #3 will share any future handling improvements
with tunnel.cc and forward.cc code.


Henrik and I seem to disagree on this being a good idea being plugged 
into BodyPipe. But as I see it BodyPipe is where Upgrade, WebSockets and 
SSL-Bump should be happening in a node which internally "receives" the 
tunnel connection and acts like a ConnStateData reading out of the tunnel.


When going DIRECT we have to assume the CONNECT is infinite length and 
handle as per now with closures.


But ... I see a potential gap in the RFC texts that when communicating 
between two proxies we can do one of several methods (Expect-100 or peer 
OPTIONS) to negotiate chunking support for any data the CONNECT has 
following it. Thus keeping the connection between the peers (only) 
re-usable after the CONNECT is finished. So long as the transformation 
(chunked encoding) is stripped away at the recipient proxy this does not 
introduce problems to the transported end-to-end data stream.


I have one client VERY interested in such otimizations across a 
multi-layer Squid cluster.


Amos

Re: ICAP connections under heavy loads

[Amos Jeffries]

On 9/09/2012 8:30 p.m., Amos Jeffries wrote:

On 9/09/2012 5:03 a.m., Alex Rousskov wrote:

On 09/07/2012 09:51 PM, Amos Jeffries wrote:

On 8/09/2012 3:17 a.m., Alex Rousskov wrote:

On 09/07/2012 08:33 AM, Alexander Komyagin wrote:
However, as I stated earlier, the comm.cc problem (actually 
semantics

problem) persists. I think it should be documented that second and
subsequent calls to comm_connect_addr() do not guarantee connection
establishment unless there was a correct select() notification.

Which to me means we cannot rely on it at all for the timeout callback
cases

Moreover, there is no need to call comm_connect_addr() in timeout cases.
The connection has timed out and should be closed. End of story. We do
not need to try extra hard and check whether the connection was
established at the OS level just when the timeout handler was scheduled;
we will not be able to detect some such cases anyway and all such cases
will be rare with any reasonable timeout.


We are agreed.

IIRC the race is made worse by our internal code going timout event 
(async) scheduling a call (async), so doubling the async queue delay. 
Which shows up worst on heavily loaded proxies.


Small correction. I mean the InProgressConnectRetry call doing that on 
the select() notification side of the race.


Amos

Re: Store_url_rewrite for squid 3+

[Amos Jeffries]

On 9/09/2012 4:19 a.m., Alex Rousskov wrote:

On 09/07/2012 09:13 PM, Amos Jeffries wrote:


Also, any revalidation requests done later must be done on the
original request URL. Not the stored URL nor the potentially different
current client request URL.

This sounds like a very important point that could justify storing the
original request URL -- exactly the kind of information I was asking
for, thank you!

Why do we have to use the original request URL for revalidation instead
of the current one? We use current, not original request headers (we do
not store the original ones), right? Is it better to combine current
headers with the original URL than it is to use the current URL with
current headers?


Revalidation requires very precise variant targeting to ensure updated 
headers received from the revalidation is not corrupting the cached 
object copy. Regardless what people may think the YouTube URLs and other 
sites being de-duplicated with store-url *are* actually pointing at 
different files on different servers with potentially different hashes 
or encoding details. Particularly in the cases where the HD and standard 
definition variants of a video are store-url mapped to the same cache 
object.


The URL and ETag are both critical details to preserve here. Also, 
anything else which is used for specific Squid->upstream identification 
of the resource being revalidated.



The store URL rewriting feature essentially assumes that any request URL
that maps to URL X is equivalent and, hence, any response to any request
URL that maps to URL X is equivalent. Why not use that assumption when
revalidating? If we receive a 304, we can keep using the stored content.
If we receive new response content, should not we assume that the stored
content [under the original URL] is stale as well?


Assumes is the right word. They are equivalent only in the proxy 
administrators thoughts. Which may be wrong or right. We have to let 
them be wrong sometimes and cause clients display problems, but we 
should not let them cause local cache corruption with revalidation 
updating cached objects meta data from incorrect variant sources.




Again, I am not trying to say that using original URL for revalidation
is wrong -- I am just trying to understand what the design constraints are.


We could simply re-fetch and store a new copy from the new client 
request details. Revalidation is an optimization, but requires correct 
identification of the particular resource and variant we have in cache. 
That goes for anything in cache, store-url is just tricky in that the 
client-side request can't present us the accurate details for server-side.




Thank you,

Alex.
P.S. The above still does not justify storing the rewritten URL(s), of
course.


No. I think those are only useful for key purposes and can be discarded 
once the object in cache is located for a HTI, or stored fro a MISS.


Amos

Re: ICAP connections under heavy loads

[Amos Jeffries]

On 9/09/2012 5:03 a.m., Alex Rousskov wrote:

On 09/07/2012 09:51 PM, Amos Jeffries wrote:

On 8/09/2012 3:17 a.m., Alex Rousskov wrote:

On 09/07/2012 08:33 AM, Alexander Komyagin wrote:

However, as I stated earlier, the comm.cc problem (actually semantics
problem) persists. I think it should be documented that second and
subsequent calls to comm_connect_addr() do not guarantee connection
establishment unless there was a correct select() notification.

Which to me means we cannot rely on it at all for the timeout callback
cases

Moreover, there is no need to call comm_connect_addr() in timeout cases.
The connection has timed out and should be closed. End of story. We do
not need to try extra hard and check whether the connection was
established at the OS level just when the timeout handler was scheduled;
we will not be able to detect some such cases anyway and all such cases
will be rare with any reasonable timeout.


We are agreed.

IIRC the race is made worse by our internal code going timout event 
(async) scheduling a call (async), so doubling the async queue delay. 
Which shows up worst on heavily loaded proxies.


The patch woudl be this:

=== modified file 'src/comm/ConnOpener.cc'
--- src/comm/ConnOpener.cc  2012-08-28 19:12:13 +
+++ src/comm/ConnOpener.cc  2012-09-09 08:20:09 +
@@ -139,6 +139,10 @@
 // it never reached fully open, so cleanup the FD handlers
 // Note that comm_close() sequence does not happen for 
partially open FD

 Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0);
+if (calls_.earlyAbort_ != NULL) {
+ calls_.earlyAbort_->cancel("Comm::ConnOpener::doneConnecting");
+calls_.earlyAbort_ = NULL;
+}
 calls_.earlyAbort_ = NULL;
 if (calls_.timeout_ != NULL) {
calls_.timeout_->cancel("Comm::ConnOpener::doneConnecting");
@@ -309,14 +313,14 @@
 doneConnecting(COMM_ERR_CLOSING, io.xerrno); // NP: is closing or 
shutdown better?

 }

-/**
+/** Timeout during connection attempt.
  * Handles the case(s) when a partially setup connection gets timed out.
- * NP: When commSetConnTimeout accepts generic CommCommonCbParams this 
can die.

  */
 void
 Comm::ConnOpener::timeout(const CommTimeoutCbParams &)
 {
-connect();
+debugs(5, 5, HERE << conn_ << ": * - ERR took too long to receive 
response.");

+doneConnecting(COMM_TIMEOUT, errno);
 }

 /* Legacy Wrapper for the retry event after COMM_INPROGRESS




The entire point of calling comm_connect_addr() from timeout callback is
to determine whether select() notification is in flight. If that fails
(as it does for your cases) it is useless doing, we may as well treat
them all as timeouts and abort quickly.

Exactly. I hope we are all in agreement that the latter is the right
thing to do. There is no point in complicating and slowing down the code
to optimize handling of rare low-level race conditions. We can handle
them by closing the connection.



I think from all this discussion the ConnOpener::DelayedConnectRetry
handler needs to be investigated as well.

Agreed. Why is that handler reusing the same failed socket? Should not
we close the failed socket descriptor upon detecting a failure and then
open a new one just before trying again?


Hmm. Not sure. This is a straight copy of the older code.

If you want a new socket FD most of what start() does needs to be 
re-done for it. Except that the start timer needs to be left unchanged, 
and the async calls need to be switched to teh new FD (would that be a 
reassignment, or a cancel + replace?)cancelled before replacing on the 
new FD.




comm_connect_addr() is called by ConnOpener in at least two cases:

1. To initiate the connection, after comm_openex in ConnOpener::start().

ConnOpener::connect() - called synchronously from start().


2. After select(2) or equivalent said that the socket is ready for
writing.

This is connect() scheduled from ConnOpener::InProgressConnectRetry().

Agreed.


  ** we could cancel the timeout and earlyAbort calls in the wrapper when
scheduling the connect() call to happen later. This will reduce the
async lag when those might kick in.

I agree that we could do that, but it will only complicate the code as
we will have to make sure that we do not delay reconnecting beyond the
timeout (which may be in 0.000 seconds), and that we re-establish the
timeout handler once we restart connecting again. I really do not think
that the increased complexity is worth occasionally eliminating one
timeout notification in an already rare failing case.

Here is the sketch for connect retries on failures, as I see it:

 default:
 ++failRetries_; // TODO: rename to failedTries

 // we failed and have to start from scratch
 ... close the temporary socket ...

 // if we exhausted all retries, bail with an error
 if (failRetries_ > Config.connect_retries) {
 ... send COMM_ERR_CONNECT to the caller ...
 return

Re: Store_url_rewrite for squid 3+

[Eliezer Croitoru]

On 09/08/2012 01:36 AM, Alex Rousskov wrote:

On 09/07/2012 01:32 PM, Eliezer Croitoru wrote:

On 09/07/2012 05:10 PM, Alex Rousskov wrote:

 I am not sure what "option" you are referring to in the above. The
 Store::get(key) API I have described is not optional -- it is the
 primary way of detecting a hit.

+1 to use the api.
are we talking about the "Store::Root().get" ?

That is where it starts, but other Store kids implement the get() method
as well IIRC. From the caller perspective, you should also look at
storeGetPublic*() and similar functions.

basic review Done already.

 The URL rewriting must happen before or during Store key calculation.

The problem is that if I am rewriting the url, unlike
url_rewrite\redirect the connection should be initiated based on the
original url and not the rewritten one.

Sure, but that is not a problem you need to worry about much. Since you
are not going to alter the original URL, the connection initiation code
will not be affected by your changes (except for some minor adjustments
if some data members get renamed or Store::get() API-related calls get
adjusted as discussed below).

Got my answers during the review.




What I do not know yet is what and where and based on what the request
is done.

For this project, you do not need to know much about connection
initiation code. You need to make a copy of the original URL, rewrite
that copy using the new helper, and put it in a new HttpRequest data
member (for example). This should probably happen (or be initiated from)
client_side.cc or client_side_request.cc.
Well after the review it seems like there is no need to copy the request 
but to use the char * storeurl.
it makes more sense since you never touch any other part of the object 
while you are getting it.

You then need to make sure that every time Store calculates a store key,
the new member (if any) is used instead of the original URL.
this is the original  reason I wanted to know\find about the initiation 
code.

but I managed to find what I need for now.


Since the current code is using the original request URL where you want
the rewritten/store URL to be used, I recommend _renaming_ all members
that keep or extract the original/request URL (e.g., MemObject::url).
This will force you to go through the relevant callers and make sure
that the right URL is used, depending on the caller context.

Exactly what I was getting into.

Once you are done, we can review whether the vast majority of changes in
the diff are a simple renaming of "url" to "requestUrl". If yes, you can
revert that renaming change. If there are many cases where old "url"
became "storeUrl" and many cases where it became "requestUrl" then the
change should stay.

Please note that the above is just a sketch. I am not suggesting any
specific new names, and I am using existing class names only as an example.

Will look into it more.
as far as I can tell most of the current code MemObject::url calls are 
from debug.






so we need to store the original url and the store_url at least for the
period of time the request is being served.

Yes, we need to keep both URLs around during the transaction lifetime.
When I was talking about storing a URL, I was only talking about Store
(i.e., storing something in the cache).
A reason I can think of is to make it possible to use 
mgr:store_url_option that will maybe give some data about in cache 
objects such as urls etc.
For most cases the access.log is what will be used but if there is a 
need it's for it it's for that.


by the way the Purge tool uses the URL part of the metadata to find urls 
and purge them in a ufs cache_dir.
since I stored objects in my cache with URL meta data as the rewritten 
one I could do lookups on the cache objects using purge.
It gives you the ability to find out exactly at this moment what objects 
urls are in the cache and in my case also rewritten ones.




(answer about storing or not the store_url in meta)

That is a separate question. I cannot answer it, unfortunately. I do not
know why Squid2 stored either of those URLs. I know Squid3 does not
store the original/request URL.
It does in ufs cahe_dir as a fact. (see the below quotation from a cache 
file)




url_rewrite api gets the http requests as the background to any action
so it cannot be used for store_url since any change to the request will
lead to the change we are trying to not do exactly.

mem_object contains:
{...
char *url;
HttpRequest *request;
...}
and the url is being used to make the hash but I still dont know what is
being taken from the mem_object to do the request.

HttpStateData::buildRequestPrefix() builds request headers. It may use
mem_object::url. I do not think you should change that code (apart from
renaming and Store::Get() API changes discussed elsewhere).

You should only be concerned with calls to storeGetPublic*() and such.
Make sure the callers do not use mem_object::url without checking
whether there is a "rewritten" st