On 19/03/11 05:41, Alex Rousskov wrote:
On 03/18/2011 12:42 AM, Amos Jeffries wrote:
------------------------------------------------------------
revno: 11299
fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3007
author: Mikio Kishi<[email protected]>
committer: Amos Jeffries<[email protected]>
branch nick: trunk
timestamp: Fri 2011-03-18 19:42:32 +1300
message:
Bug 3007: CONNECT to cache_peer returns 000 status code
modified:
src/tunnel.cc
=== modified file 'src/tunnel.cc'
--- a/src/tunnel.cc 2010-11-28 01:15:27 +0000
+++ b/src/tunnel.cc 2011-03-18 06:42:32 +0000
@@ -526,6 +526,9 @@
static void
tunnelProxyConnectedWriteDone(int fd, char *buf, size_t size, comm_err_t
flag, int xerrno, void *data)
{
+ TunnelStateData *tunnelState = static_cast<TunnelStateData *>(data);
+ debugs(26, 3, HERE<< "FD "<< fd<< " tunnelState="<< tunnelState);
+ *tunnelState->status_ptr = HTTP_OK;
tunnelConnectedWriteDone(fd, buf, size, flag, xerrno, data);
}
Seems wrong to set HTTP_OK even if the write failed. Besides, we have
already set tunnelState->status_ptr to HTTP_OK right before scheduling
the above callback:
Nope. Two paths exist, status_ptr is/was not set at all in the peering
one. Thus the (default) 000 status in the log.
You are right about the missing write() fail handling though. Thanks for
that. Looking at it closely I think this is the cause of the CONNECT
requests not doing failover between peers if connect to one fails.
tunnelConnectedWriteDone calls tunnelErrorComplete and terminates the
whole transaction on a failure.
When tunnelProxyConnectedWriteDone receives a failure flags however it
just means the peer is unusable, we are entirely able to abort that peer
link and re-forward to other peers or direct.
I'm away for a few days now, but will look at this when I'm back.
static void
tunnelConnected(int fd, void *data)
{
TunnelStateData *tunnelState = (TunnelStateData *)data;
debugs(26, 3, "tunnelConnected: FD "<< fd<< " tunnelState="<<
tunnelState);
*tunnelState->status_ptr = HTTP_OK;
AsyncCall::Pointer call = commCbCall(5,5, "tunnelConnectedWriteDone",
CommIoCbPtrFun(tunnelConnectedWriteDone, tunnelState));
Comm::Write(tunnelState->client.fd(), conn_established,
strlen(conn_established), call, NULL);
}
Why do we need to set HTTP_OK twice?
There are two paths, one when connecting DIRECT:
(tunnelConnectDone -> tunnelConnected -> tunnelConnectedWriteDone)
and one when relaying to a cache_peer
(tunnelConnectDone -> tunnelProxyConnected ->
tunnelProxyConnectedWriteDone -> tunnelConnectedWriteDone)
We could perhapse set HTTP_OK in tunnelConnectDone before deciding which
to take. Then update it again in tunnelProxyConnectedWriteDone on write
failure.
I kind of think setting it like this after the connect has actually
been done is better. But either way is doable.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.11
Beta testers wanted for 3.2.0.5
