Re: /bzr/squid3/trunk/ r11299: Bug 3007: CONNECT to cache_peer returns 000 status code

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<mkishi_at_104.net>
>> committer: Amos Jeffries<squid3_at_treenet.co.nz>
>> 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